Split GuardField into GuardFieldType and GuardFieldLength instructions.

Split GuardField into GuardFieldType and GuardFieldLength instructions.

Record expected length offset that matches guarded_cid.

This greatly simplifies code generation and control flow inside these guards.

Fixes the bug that was causing unoptimized code to call into runtime even when guard did not fail.

Add --trace-field-guards flags to trace updates to the state of guarded fields' properties.

BUG=http://dartbug.com/19003
R=fschneider@google.com, johnmccutchan@google.com, zra@google.com

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

[Vyacheslav Egorov (Google), 2014-05-28 13:48:15]

PTAL

This is an ia32 version. I will port if structure looks ok to you.

[Cutch, 2014-05-28 14:13:31]

Structure lgtm. Some other comments.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1535: // Jump when class id guard and
list length guard are okay.
remove list length from comment

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1648: __ cmpl(offset_reg,
Immediate(0));
move the compare / jmp above the load of length_reg?

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object_sn...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object_sn...
runtime/vm/raw_object_snapshot.cc:810:
writer->WriteIntptrValue(ptr()->guarded_list_length_in_object_offset_);
Do we want to write this? The offset isn't compatible between 32-bit and 64-bit
platforms, right?

[Florian Schneider, 2014-05-28 14:57:36]

LGTM with comments. Go ahead with port to the other archs.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3180: Isolate::Current()->GetNextDeoptId());
Why get a new deopt-id? The Instruction constructor allocates one for it
already.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3187: Isolate::Current()->GetNextDeoptId());
Why get a new deopt-id? The Instruction constructor allocates one for it
already.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:260: instr->IsGuardFieldLength() ||
Can we just change CanBecomeDeoptimizationTarget() to return true for
GuardField?

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1455: bool opt) const {
Space alignment.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1539: if
(!field().needs_length_check()) {
Maybe add a comment that the GuardFieldLength is expected to occur _after_ the
GuardFieldCid, and they must not be reordered.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1627: if (field().guarded_list_length()
== Field::kNoFixedLength) {
ASSERT(!compiler->is_optimizing())?

In optimizing mode, this guard should be eliminated in this case.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1648: __ cmpl(offset_reg,
Immediate(0));
Maybe 

__ cmpl(offset_reg, Immediate(kUnknownLengthOffset));
__ j(EQUAL, &ok)

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1665: 
ASSERT(field().guarded_list_length_in_object_offset() != kUnknownLengthOffset)

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

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:6778: result.set_guarded_list_length_in_object_offset(-1);
s/-1/kUnknownLengthOffset/

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7098: set_guarded_list_length_in_object_offset(-1);
s/-1/kUnknownLengthOffset/

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7125: set_guarded_list_length_in_object_offset(-1);
s/-1/kUnknownLengthOffset/

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

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:688: intptr_t guarded_list_length_in_object_offset_;
Make this smaller (int8_t or int16_t) so that the following byte does not make
the object larger by a whole word-size.

[Florian Schneider, 2014-05-28 14:57:37]

LGTM with comments. Go ahead with port to the other archs.

[Florian Schneider, 2014-05-28 14:58:52]

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3180: Isolate::Current()->GetNextDeoptId());
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Why get a new deopt-id? The Instruction constructor allocates one for it
> already.

Never mind this comment. It's needed because we use the share this instruction
between optimizing/unoptimizing compiler.

[Vyacheslav Egorov (Google), 2014-05-29 17:36:51]

Comments addressed. Ports finished. 

Adding Zach for ARM/ARM64/MIPS

Please take another look guys.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:260: instr->IsGuardFieldLength() ||
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Can we just change CanBecomeDeoptimizationTarget() to return true for
> GuardField?

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1455: bool opt) const {
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Space alignment.

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1535: // Jump when class id guard and
list length guard are okay.
On 2014/05/28 14:13:31, Cutch wrote:
> remove list length from comment

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1539: if
(!field().needs_length_check()) {
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Maybe add a comment that the GuardFieldLength is expected to occur _after_ the
> GuardFieldCid, and they must not be reordered.

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1627: if (field().guarded_list_length()
== Field::kNoFixedLength) {
On 2014/05/28 14:57:36, Florian Schneider wrote:
> ASSERT(!compiler->is_optimizing())?
> 
> In optimizing mode, this guard should be eliminated in this case.

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1648: __ cmpl(offset_reg,
Immediate(0));
On 2014/05/28 14:13:31, Cutch wrote:
> move the compare / jmp above the load of length_reg?

I am doing some manual scheduling here. Keeping loads together to offset
latency.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1648: __ cmpl(offset_reg,
Immediate(0));
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Maybe 
> 
> __ cmpl(offset_reg, Immediate(kUnknownLengthOffset));
> __ j(EQUAL, &ok)

I could, but on MIPS I would have to load immediate into a register then. So for
having platforms aligned I better have comparison with 0.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1665: 
On 2014/05/28 14:57:36, Florian Schneider wrote:
> ASSERT(field().guarded_list_length_in_object_offset() != kUnknownLengthOffset)

Done.

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

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:6778: result.set_guarded_list_length_in_object_offset(-1);
On 2014/05/28 14:57:36, Florian Schneider wrote:
> s/-1/kUnknownLengthOffset/

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7098: set_guarded_list_length_in_object_offset(-1);
On 2014/05/28 14:57:36, Florian Schneider wrote:
> s/-1/kUnknownLengthOffset/

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7125: set_guarded_list_length_in_object_offset(-1);
On 2014/05/28 14:57:36, Florian Schneider wrote:
> s/-1/kUnknownLengthOffset/

Done.

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

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:688: intptr_t guarded_list_length_in_object_offset_;
On 2014/05/28 14:57:36, Florian Schneider wrote:
> Make this smaller (int8_t or int16_t) so that the following byte does not make
> the object larger by a whole word-size.

Done.

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object_sn...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/304703002/diff/20001/runtime/vm/raw_object_sn...
runtime/vm/raw_object_snapshot.cc:810:
writer->WriteIntptrValue(ptr()->guarded_list_length_in_object_offset_);
On 2014/05/28 14:13:31, Cutch wrote:
> Do we want to write this? The offset isn't compatible between 32-bit and
64-bit
> platforms, right?

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (left):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1912: __ CompareImmediate(value_cid_reg,
field_length);
It seems on ARM this code always contained a bug: value_cid_reg contains Smi
which is compared here to an integer value so this guard always fails.

[zra, 2014-05-29 18:20:33]

arm64/arm/mips lgtm

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1650: Register value_reg =
locs()->in(0).reg();
const Register

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1652: Register value_cid_reg =
needs_value_cid_temp_reg ?
const Register

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1788: Register value_reg =
locs()->in(0).reg();
ditto

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1792: Register field_reg =
locs()->temp(0).reg();
ditto

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1793: Register offset_reg =
locs()->temp(1).reg();
ditto

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1794: Register length_reg =
locs()->temp(2).reg();
ditto

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1409: Register value_reg =
locs()->in(0).reg();
const Register here and below.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1530:
summary->AddTemp(Location::RequiresRegister());
arm64 has a second assembler temporary TMP2 that you can use to avoid allocating
a temp, here, I think.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:1517: Register value_reg =
locs()->in(0).reg();
const Register here and below.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:1669: __ lw(CMPRES2,
FieldAddress(field_reg,
You can move this load to the branch delay slot of the following blez.

[Vyacheslav Egorov (Google), 2014-06-02 10:48:37]

PTAL

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1650: Register value_reg =
locs()->in(0).reg();
On 2014/05/29 18:20:34, zra wrote:
> const Register

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1652: Register value_cid_reg =
needs_value_cid_temp_reg ?
On 2014/05/29 18:20:34, zra wrote:
> const Register

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1788: Register value_reg =
locs()->in(0).reg();
On 2014/05/29 18:20:34, zra wrote:
> ditto

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1792: Register field_reg =
locs()->temp(0).reg();
On 2014/05/29 18:20:34, zra wrote:
> ditto

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1793: Register offset_reg =
locs()->temp(1).reg();
On 2014/05/29 18:20:34, zra wrote:
> ditto

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1794: Register length_reg =
locs()->temp(2).reg();
On 2014/05/29 18:20:34, zra wrote:
> ditto

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1409: Register value_reg =
locs()->in(0).reg();
On 2014/05/29 18:20:34, zra wrote:
> const Register here and below.

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1530:
summary->AddTemp(Location::RequiresRegister());
On 2014/05/29 18:20:34, zra wrote:
> arm64 has a second assembler temporary TMP2 that you can use to avoid
allocating
> a temp, here, I think.

I would prefer not to use it for long living values inside the function because
it is used in unexpected ways inside instruction in assembler.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:1517: Register value_reg =
locs()->in(0).reg();
On 2014/05/29 18:20:34, zra wrote:
> const Register here and below.

Done.

https://codereview.chromium.org/304703002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:1669: __ lw(CMPRES2,
FieldAddress(field_reg,
On 2014/05/29 18:20:34, zra wrote:
> You can move this load to the branch delay slot of the following blez.

I just moved it to right after the branch. There is no need to load this value
when the branch is taken I am just trying to do some manual scheduling.

[Florian Schneider, 2014-06-02 11:37:00]

lgtm

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1698: __ CompareImmediate(IP,
kIllegalCid);
Maybe use cmp instead of CompareImmediate which may use IP for the immediate
itself. - Or insert an COMPILE_ASSERT about the size of kIllegalCid.

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1838: __ CompareImmediate(IP,
Smi::RawValue(field().guarded_list_length()));
Use cmp here. What is the max guarded list length? CompareImmediate uses IP
itself if field().guarded_list_length() is larger than the max immediate operand
for cmp.

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1458: __ ldr(TMP, field_cid_operand);
General suggestion for ARM/ARM64/MIPS code (You don't need to change this in
this CL):

It's always a little brittle to use the assembler temporaries for the generated
code. Here it's ok because CompareImmediate uses TMP2 (and kIllegalCid is small
enough).

I'd rather forbid using them from outside, or make them allocatable and pass a
temp explicitly to CompareImmedate, LoadImmediate, AddImmediate, etc.

[Vyacheslav Egorov (Google), 2014-06-02 12:09:46]

Thanks for the review. Landing.

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:1838: __ CompareImmediate(IP,
Smi::RawValue(field().guarded_list_length()));
On 2014/06/02 11:37:01, Florian Schneider wrote:
> Use cmp here. What is the max guarded list length? CompareImmediate uses IP
> itself if field().guarded_list_length() is larger than the max immediate
operand
> for cmp.

Good catch. Switched to allocating a register for now.

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/304703002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:1458: __ ldr(TMP, field_cid_operand);
> I'd rather forbid using them from outside, or make them allocatable and pass a
> temp explicitly to CompareImmedate, LoadImmediate, AddImmediate, etc.

I second this. The stuff we are writing now is very brittle. In theory
completely forbidding using them from outside will lead to very strange and
ineffecient code sometimes. 

I think instead we need a real lowering pass that breaks our huge monolithic
instructions even further exposing which kinds of temps they need and so on.

[Vyacheslav Egorov (Chromium), 2014-06-02 13:54:43]

Committed patchset #5 manually as r36872 (presubmit successful).