Tests for GuardField length check along with bug fixes

Tests for GuardField length check along with bug fixes

R=regis@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27279

[srdjan, 2013-08-26 16:13:38]

Please update/expand the description of the CL (final fields change). Please 
verify that no excessive deoptimization occur because the optimization is
applied to any field, not only finals .

https://codereview.chromium.org/22915008/diff/4001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/22915008/diff/4001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1856: static intptr_t GetListLength(const Object&
value) {
It is error-prone to pass around the raw value of Smi. Better would be to return
RawSmi*, or to return integer and warp it into the Smi at call site. 

The structure of class RawField indicates that an integer not a Smi (tagged)
should be stored into guarded_list_length_.

[Cutch, 2013-08-26 17:28:31]

On 2013/08/26 16:13:38, srdjan wrote:
> Please update/expand the description of the CL (final fields change). Please 
> verify that no excessive deoptimization occur because the optimization is
> applied to any field, not only finals .
> 
>
https://codereview.chromium.org/22915008/diff/4001/runtime/vm/code_generator.cc
> File runtime/vm/code_generator.cc (right):
> 
>
https://codereview.chromium.org/22915008/diff/4001/runtime/vm/code_generator....
> runtime/vm/code_generator.cc:1856: static intptr_t GetListLength(const Object&
> value) {
> It is error-prone to pass around the raw value of Smi. Better would be to
return
> RawSmi*, or to return integer and warp it into the Smi at call site. 
> 
> The structure of class RawField indicates that an integer not a Smi (tagged)
> should be stored into guarded_list_length_.

Hi,

This CL is not ready for review because I expanded it beyond the original change
when I discovered a bug.

Sorry I forgot I sent it for review. Jet lag.

John

[Cutch, 2013-08-26 17:57:44]

On 2013/08/26 16:13:38, srdjan wrote:
> Please update/expand the description of the CL (final fields change). Please 
> verify that no excessive deoptimization occur because the optimization is
> applied to any field, not only finals .
> 

The optimization is still only applied to final fields. I moved the logic
because I discovered that (non-final) fields initialized in place could be
flagged for the optimization.

[Cutch, 2013-08-27 10:35:29]

PTAL at P.S. 3.

[srdjan, 2013-08-27 17:45:22]

The comments in *_arm* file applies to other architectures as well.

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1540: const intptr_t field_length =
Smi::RawValue(field().guarded_list_length());
It is not a good idea to hang onto tagged value as integer, unless the name
describes it, e.g., tagged_filed_length. Instead, I would do
Smi::New(field_length) wherever you need to pass length as Smi.

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1622: __ CompareImmediate(value_cid_reg,
kNullCid);
This code seems identical to the one below, please factor out.

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:685: return
reinterpret_cast<RawObject**>(&ptr()->dependent_code_);
To be correct, 'to()' should go up to guarded_list_length_ instead of to
dependent_code_ only.

[Cutch, 2013-08-28 12:24:59]

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1540: const intptr_t field_length =
Smi::RawValue(field().guarded_list_length());
On 2013/08/27 17:45:22, srdjan wrote:
> It is not a good idea to hang onto tagged value as integer, unless the name
> describes it, e.g., tagged_filed_length. Instead, I would do
> Smi::New(field_length) wherever you need to pass length as Smi.

Thanks for this.

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1622: __ CompareImmediate(value_cid_reg,
kNullCid);
On 2013/08/27 17:45:22, srdjan wrote:
> This code seems identical to the one below, please factor out.

This code is not identical to the block below. This block does the following:

1. Determines what type of list we are dealing with.
2. Compares the guarded field length with the list length.

The code below does the following:

1. Determine what type of list we are dealing with.
2. Sets the guarded field length for the first time.

What happens is that when we compile the function we have no static knowledge
(field_cid and value_cid are unknown). The first time it executes we set the
field length for the first time and the second time we execute it we must guard
against it. Future compilations of the function should have static knowledge
(field_cid or value_cid will be known) and emit much smaller code.

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/22915008/diff/11001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:685: return
reinterpret_cast<RawObject**>(&ptr()->dependent_code_);
On 2013/08/27 17:45:22, srdjan wrote:
> To be correct, 'to()' should go up to guarded_list_length_ instead of to
> dependent_code_ only.

Done.

[srdjan, 2013-08-28 16:00:55]

Also CC-d zra and regis to go over ARM/MIPS code.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1680: Label check_array, local_exit,
local_fail;
Optional: maybe rename the labels to be self-explanatory:
local_exit -> length_loaded
local_fail -> no_fixed_length

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1680: Label check_array, local_exit,
local_fail;
Please remind me how can field_cid be kIllegalCid but the length is known?

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1692: __ cmpl(field_length_operand,
value_cid_reg);
Eliminate push-pop, use cmpl(field_length, FieldAddress())
Also instead of jump to local_exit, you can do j(NOT_EQUAL, fail).

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1704: __ cmpl(field_length_operand,
value_cid_reg);
Use __ cmpl(field_length_operand, FieldAddress(...)) and avoid push/pop,
followed by j(NOT_EQUAL, fail)

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1741: __ cmpl(value_cid_reg,
Immediate(Smi::RawValue(field_length)));
Eliminate push/pop by using __cmpl(FieldAddress(...), Immediate(...));

Also below.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1747: __ cmpl(value_cid_reg,
Immediate(Smi::RawValue(field_length)));
Eliminate push/pop

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:1639: ASSERT(field_cid == kIllegalCid);
Similar comments as in ia32

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:5353: result.set_is_nullable(false);
Maybe add comment that list length guard is implemented for final variables
only, at the moment.

[regis, 2013-08-28 16:06:34]

ARM/MIPS LGTM

[Cutch, 2013-08-28 19:20:36]

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1680: Label check_array, local_exit,
local_fail;
On 2013/08/28 16:00:55, srdjan wrote:
> Please remind me how can field_cid be kIllegalCid but the length is known?

field_cid is kIllegalCid and field_length is kUnknownLength at compile time but
this code path can be executed after updating field_cid_operand and
field_length_operand but before we recompile with the new knowledge. At run-time
the following happens:

First time we execute:
1. We jump at 1663 because we field_cid_operand is kIlllegalCid.
2. We end up at 1768 and initialized field_cid_operand and field_length_operand
for the first time.

Second time we execute:
1. We fall through at 1663 because field_cid_operand matches value_cid_reg.
2. We check that field_length_operand matches the length of the list, if not we
set field_length_operand to kNoFixedLength and end up at fail.

[Cutch, 2013-08-29 08:27:17]

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1680: Label check_array, local_exit,
local_fail;
On 2013/08/28 16:00:55, srdjan wrote:
> Optional: maybe rename the labels to be self-explanatory:
> local_exit -> length_loaded
> local_fail -> no_fixed_length

Done.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1692: __ cmpl(field_length_operand,
value_cid_reg);
On 2013/08/28 16:00:55, srdjan wrote:
> Eliminate push-pop, use cmpl(field_length, FieldAddress())

We do not know field_length at compile time, I've added an ASSERT to this
effect.

> Also instead of jump to local_exit, you can do j(NOT_EQUAL, fail).

If we j(NOT_EQUAL, fail) when the list length is equal we fall through into code
which will destroy the known list length.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1704: __ cmpl(field_length_operand,
value_cid_reg);
On 2013/08/28 16:00:55, srdjan wrote:
> Use __ cmpl(field_length_operand, FieldAddress(...)) and avoid push/pop,
> followed by j(NOT_EQUAL, fail)

field_length_operand is a FieldAddress there is no cmpl(FieldAddress,
FieldAddress).

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1741: __ cmpl(value_cid_reg,
Immediate(Smi::RawValue(field_length)));
On 2013/08/28 16:00:55, srdjan wrote:
> Eliminate push/pop by using __cmpl(FieldAddress(...), Immediate(...));
> 
> Also below.

Done.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1747: __ cmpl(value_cid_reg,
Immediate(Smi::RawValue(field_length)));
On 2013/08/28 16:00:55, srdjan wrote:
> Eliminate push/pop

Done.

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:5353: result.set_is_nullable(false);
On 2013/08/28 16:00:55, srdjan wrote:
> Maybe add comment that list length guard is implemented for final variables
> only, at the moment.

Done.

[srdjan, 2013-08-29 16:05:25]

LGTM

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22915008/diff/18001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1692: __ cmpl(field_length_operand,
value_cid_reg);
On 2013/08/29 08:27:17, Cutch wrote:
> On 2013/08/28 16:00:55, srdjan wrote:
> > Eliminate push-pop, use cmpl(field_length, FieldAddress())
> 
> We do not know field_length at compile time, I've added an ASSERT to this
> effect.
> 
> > Also instead of jump to local_exit, you can do j(NOT_EQUAL, fail).
> 
> If we j(NOT_EQUAL, fail) when the list length is equal we fall through into
code
> which will destroy the known list length.

I see: field_length_operand is an address, not a register.

******

Currently the code does:

__ cmpl (..)
__ popl(...)
__ jmp(&local_exit);

...
__ Bind(&local_exit);
__ j(NOT_EQUAL, fail);
...

I thought could be safely transformed to:

__ cmpl(...)
__ popl(..)
__ jmp(NOT_EQUAL, &fail).

Also try using NearLabels if possible, it shortens the code.

[Cutch, 2013-09-08 18:16:20]

On 2013/08/29 16:05:25, srdjan wrote:
> Currently the code does:
> 
> __ cmpl (..)
> __ popl(...)
> __ jmp(&local_exit);
> 
> ...
> __ Bind(&local_exit);
> __ j(NOT_EQUAL, fail);
> ...
> 
> I thought could be safely transformed to:
> 
> __ cmpl(...)
> __ popl(..)
> __ jmp(NOT_EQUAL, &fail).
> 

I would need to also:

__ jmp(EQUAL, &local_exit);

Because the code is also detecting the type of the list we are seeing and
eventually falls through into a "not a fixed length list class id" trap which
sets the guarded list length to kNoFixedLength.

> Also try using NearLabels if possible, it shortens the code.

I have successfully switched to near labels so that short jumps are executed.

Note: This CL also includes a fix for the ASSERT failure in
https://code.google.com/p/dart/issues/detail?id=7259. The ASSERT should have
been inside if (field_has_length) { .. } at which point we know the register has
been allocated.

[Cutch, 2013-09-08 21:45:22]

Committed patchset #7 manually as r27279 (presubmit successful).