Initial support for length guards on final fields.

Initial support for length guards on final fields.

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

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

[Cutch, 2013-08-13 20:45:20]

PTAL- I've only included the IA32 changes. Once I know you are happy with the
structure I will port the changes to x64,arm, and mips.

[srdjan, 2013-08-14 17:01:08]

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

https://codereview.chromium.org/22851003/diff/1/runtime/vm/code_generator.cc#...
runtime/vm/code_generator.cc:1867: intptr_t list_length = Field::kNoLength;
You can eliminate list_length variable and return directly from each individual
case.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/code_generator.cc#...
runtime/vm/code_generator.cc:1904: }
ASSERT(!field.is_final) ?

https://codereview.chromium.org/22851003/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/22851003/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:2901: // that the field doesn't have a length.
Maybe add a 'why' to the comment, or a "for now".

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1581: const bool field_has_length =
field_length >= Field::kUnknownLength;
You can move this 'intelligence' into class Field: has_length() const { return
... }

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1582: const bool need_value_temp_reg =
Maybe assert that only final fields have length. That means that the extra code
is emitted only in the constructors and should not contribute significantly to
code growth,

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1602: const bool need_value_temp_reg =
s/need_value_temp_reg/needs_value_temp_reg/ ?

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1605: const bool need_field_temp_reg =
ditto ?

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1669: // Field guard may have
remembered list length, checked it.
s/checked/check/

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1682: }
else ?

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1718: }
else ?

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1734: if (value_cid == kDynamicCid) {
Maybe do not emit extra code if field is not final?

[Cutch, 2013-08-15 16:39:12]

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

https://codereview.chromium.org/22851003/diff/1/runtime/vm/code_generator.cc#...
runtime/vm/code_generator.cc:1867: intptr_t list_length = Field::kNoLength;
On 2013/08/14 17:01:08, srdjan wrote:
> You can eliminate list_length variable and return directly from each
individual
> case.

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/22851003/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:2901: // that the field doesn't have a length.
On 2013/08/14 17:01:08, srdjan wrote:
> Maybe add a 'why' to the comment, or a "for now".

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1581: const bool field_has_length =
field_length >= Field::kUnknownLength;
On 2013/08/14 17:01:08, srdjan wrote:
> You can move this 'intelligence' into class Field: has_length() const { return
> ... }

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1582: const bool need_value_temp_reg =
On 2013/08/14 17:01:08, srdjan wrote:
> Maybe assert that only final fields have length. That means that the extra
code
> is emitted only in the constructors and should not contribute significantly to
> code growth,

The Field::needs_length_check function now includes an assertion check.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1602: const bool need_value_temp_reg =
On 2013/08/14 17:01:08, srdjan wrote:
> s/need_value_temp_reg/needs_value_temp_reg/ ?

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1605: const bool need_field_temp_reg =
On 2013/08/14 17:01:08, srdjan wrote:
> ditto ?

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1669: // Field guard may have
remembered list length, checked it.
On 2013/08/14 17:01:08, srdjan wrote:
> s/checked/check/

Done.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1682: }
On 2013/08/14 17:01:08, srdjan wrote:
> else ?

Good catch.

https://codereview.chromium.org/22851003/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1734: if (value_cid == kDynamicCid) {
On 2013/08/14 17:01:08, srdjan wrote:
> Maybe do not emit extra code if field is not final?

Done.

[srdjan, 2013-08-16 22:26:01]

LGTM with comments

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

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1876: return Field::kNoLength;
It may be easier to understand if renamed to kNoFixedLength, kUnknownFixedLength
, since growable array has length.

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h#newcod...
runtime/vm/object.h:1987: // If length is kVariableLength this field has
multiple list lengths
kVariableLength?

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h#newcod...
runtime/vm/object.h:2000: ASSERT((r && is_final()) || !r);
This can be simpler: !r || is_final()

[Cutch, 2013-08-19 21:52:01]

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

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1876: return Field::kNoLength;
On 2013/08/16 22:26:01, srdjan wrote:
> It may be easier to understand if renamed to kNoFixedLength,
kUnknownFixedLength
> , since growable array has length.

Done.

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h#newcod...
runtime/vm/object.h:1987: // If length is kVariableLength this field has
multiple list lengths
On 2013/08/16 22:26:01, srdjan wrote:
> kVariableLength?

Done.

https://codereview.chromium.org/22851003/diff/9001/runtime/vm/object.h#newcod...
runtime/vm/object.h:2000: ASSERT((r && is_final()) || !r);
On 2013/08/16 22:26:01, srdjan wrote:
> This can be simpler: !r || is_final()

Done.

[Cutch, 2013-08-20 20:58:52]

Zach & Regis PTAL at MIPS and ARM changes.

[regis, 2013-08-20 21:24:04]

LGTM with one detail.

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

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1608: __ Push(value_cid_reg);
Instead of all the push/pop pairs below, can you allocate one more temp register
and use it?

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1666: __ Push(value_cid_reg);
Same comment as for ARM.

[zra, 2013-08-20 21:40:51]

arm/mips lgtm

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1666: __ Push(value_cid_reg);
Should just be able to use TMP to save/restore value_cid_reg here and below.

[Cutch, 2013-08-20 22:48:51]

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

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:1608: __ Push(value_cid_reg);
On 2013/08/20 21:24:04, regis wrote:
> Instead of all the push/pop pairs below, can you allocate one more temp
register
> and use it?

Done.

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1666: __ Push(value_cid_reg);
On 2013/08/20 21:24:04, regis wrote:
> Same comment as for ARM.

Done.

https://codereview.chromium.org/22851003/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1666: __ Push(value_cid_reg);
On 2013/08/20 21:40:51, zra wrote:
> Should just be able to use TMP to save/restore value_cid_reg here and below.

Done.

[regis, 2013-08-20 22:55:09]

LGTM

[Cutch, 2013-08-21 15:32:54]

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