Split GuardField into GuardFieldType and GuardFieldLength instructions.

runtime/vm/intermediate_language_ia32.cc

Index: runtime/vm/intermediate_language_ia32.cc
diff --git a/runtime/vm/intermediate_language_ia32.cc b/runtime/vm/intermediate_language_ia32.cc
index 5b6f8b850a7c19484a8f8198e6ce0302c47a00ff..7f416b476f66b3a5e6bd8c3e4c095f2b872700b7 100644
--- a/runtime/vm/intermediate_language_ia32.cc
+++ b/runtime/vm/intermediate_language_ia32.cc
@@ -1451,52 +1451,54 @@ void StoreIndexedInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 }
 
 
-LocationSummary* GuardFieldInstr::MakeLocationSummary(Isolate* isolate,
+LocationSummary* GuardFieldClassInstr::MakeLocationSummary(Isolate* isolate,
                                                       bool opt) const {

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

Space alignment.

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

Done.

   const intptr_t kNumInputs = 1;
   LocationSummary* summary = new(isolate) LocationSummary(
       isolate, kNumInputs, 0, LocationSummary::kNoCall);
   summary->set_in(0, Location::RequiresRegister());
-  const bool field_has_length = field().needs_length_check();
-  const bool need_value_temp_reg =
-      (field_has_length || ((value()->Type()->ToCid() == kDynamicCid) &&
-                            (field().guarded_cid() != kSmiCid)));
-  if (need_value_temp_reg) {
+
+  const intptr_t value_cid = value()->Type()->ToCid();
+  const intptr_t field_cid = field().guarded_cid();
+
+  const bool emit_full_guard = !opt || (field_cid == kIllegalCid);
+  const bool needs_value_cid_temp_reg =
+    (value_cid == kDynamicCid) && (emit_full_guard || (field_cid != kSmiCid));
+  const bool needs_field_temp_reg = emit_full_guard;
+
+  if (needs_value_cid_temp_reg) {
     summary->AddTemp(Location::RequiresRegister());
   }
-  const bool need_field_temp_reg =
-      field_has_length || (field().guarded_cid() == kIllegalCid);
-  if (need_field_temp_reg) {
+
+  if (needs_field_temp_reg) {
     summary->AddTemp(Location::RequiresRegister());
   }
+
   return summary;
 }
 
 
-void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+void GuardFieldClassInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+  const intptr_t value_cid = value()->Type()->ToCid();
   const intptr_t field_cid = field().guarded_cid();
   const intptr_t nullability = field().is_nullable() ? kNullCid : kIllegalCid;
-  const intptr_t field_length = field().guarded_list_length();
-  const bool field_has_length = field().needs_length_check();
-  const bool needs_value_temp_reg =
-      (field_has_length || ((value()->Type()->ToCid() == kDynamicCid) &&
-                            (field().guarded_cid() != kSmiCid)));
-  const bool needs_field_temp_reg =
-      field_has_length || (field().guarded_cid() == kIllegalCid);
-  if (field_has_length) {
-    // Currently, we should only see final fields that remember length.
-    ASSERT(field().is_final());
-  }
 
   if (field_cid == kDynamicCid) {
     ASSERT(!compiler->is_optimizing());
     return;  // Nothing to emit.
   }
-  const intptr_t value_cid = value()->Type()->ToCid();
+
+  const bool emit_full_guard =
+    !compiler->is_optimizing() || (field_cid == kIllegalCid);
+
+  const bool needs_value_cid_temp_reg =
+    (value_cid == kDynamicCid) && (emit_full_guard || (field_cid != kSmiCid));
+
+  const bool needs_field_temp_reg = emit_full_guard;
 
   Register value_reg = locs()->in(0).reg();
 
-  Register value_cid_reg = needs_value_temp_reg ?
+  Register value_cid_reg = needs_value_cid_temp_reg ?
       locs()->temp(0).reg() : kNoRegister;
 
   Register field_reg = needs_field_temp_reg ?
@@ -1509,98 +1511,17 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 
   Label* fail = (deopt != NULL) ? deopt : &fail_label;
 
-  if (!compiler->is_optimizing() || (field_cid == kIllegalCid)) {
-    if (!compiler->is_optimizing() && (field_reg == kNoRegister)) {
-      // Currently we can't have different location summaries for optimized
-      // and non-optimized code. So instead we manually pick up a register
-      // that is known to be free because we know how non-optimizing compiler
-      // allocates registers.
-      field_reg = EBX;
-      ASSERT((field_reg != value_reg) && (field_reg != value_cid_reg));
-    }
-
+  if (emit_full_guard) {
     __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
 
     FieldAddress field_cid_operand(field_reg, Field::guarded_cid_offset());
     FieldAddress field_nullability_operand(
         field_reg, Field::is_nullable_offset());
-    FieldAddress field_length_operand(
-        field_reg, Field::guarded_list_length_offset());
 
     if (value_cid == kDynamicCid) {
-      if (value_cid_reg == kNoRegister) {
-        ASSERT(!compiler->is_optimizing());
-        value_cid_reg = EDX;
-        ASSERT((value_cid_reg != value_reg) && (field_reg != value_cid_reg));
-      }
-
       LoadValueCid(compiler, value_cid_reg, value_reg);
-      Label skip_length_check;
       __ cmpl(value_cid_reg, field_cid_operand);
-      // Value CID != Field guard CID, skip length check.
-      __ j(NOT_EQUAL, &skip_length_check);
-      if (field_has_length) {
-        // Field guard may have remembered list length, check it.
-        if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
-          __ pushl(value_cid_reg);
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, Array::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));
-          __ popl(value_cid_reg);
-        } else if (RawObject::IsTypedDataClassId(field_cid)) {
-          __ pushl(value_cid_reg);
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, TypedData::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));
-          __ popl(value_cid_reg);
-        } else {
-          ASSERT(field_cid == kIllegalCid);
-          ASSERT(field_length == Field::kUnknownFixedLength);
-          // At compile time we do not know the type of the field nor its
-          // length. At execution time we may have set the class id and
-          // list length so we compare the guarded length with the
-          // list length here, without this check the list length could change
-          // without triggering a deoptimization.
-          Label check_array, length_compared, no_fixed_length;
-          // If length is negative the length guard is either disabled or
-          // has not been initialized, either way it is safe to skip the
-          // length check.
-          __ cmpl(field_length_operand, Immediate(Smi::RawValue(0)));
-          __ j(LESS, &skip_length_check);
-          __ cmpl(value_cid_reg, Immediate(kNullCid));
-          __ j(EQUAL, &no_fixed_length, Assembler::kNearJump);
-          // Check for typed data array.
-          __ cmpl(value_cid_reg, Immediate(kTypedDataInt32x4ArrayCid));
-          // Not a typed array or a regular array.
-          __ j(GREATER, &no_fixed_length, Assembler::kNearJump);
-          __ cmpl(value_cid_reg, Immediate(kTypedDataInt8ArrayCid));
-          // Could still be a regular array.
-          __ j(LESS, &check_array, Assembler::kNearJump);
-          __ pushl(value_cid_reg);
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, TypedData::length_offset()));
-          __ cmpl(field_length_operand, value_cid_reg);
-          __ popl(value_cid_reg);
-          __ jmp(&length_compared, Assembler::kNearJump);
-          // Check for regular array.
-          __ Bind(&check_array);
-          __ cmpl(value_cid_reg, Immediate(kImmutableArrayCid));
-          __ j(GREATER, &no_fixed_length, Assembler::kNearJump);
-          __ cmpl(value_cid_reg, Immediate(kArrayCid));
-          __ j(LESS, &no_fixed_length, Assembler::kNearJump);
-          __ pushl(value_cid_reg);
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, Array::length_offset()));
-          __ cmpl(field_length_operand, value_cid_reg);
-          __ popl(value_cid_reg);
-          __ jmp(&length_compared, Assembler::kNearJump);
-          __ Bind(&no_fixed_length);
-          __ jmp(fail);
-          __ Bind(&length_compared);
-        }
-        __ j(NOT_EQUAL, fail);
-      }
-      __ Bind(&skip_length_check);
+      __ j(EQUAL, &ok);
       __ cmpl(value_cid_reg, field_nullability_operand);
     } else if (value_cid == kNullCid) {
       // Value in graph known to be null.
@@ -1608,110 +1529,38 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
       __ cmpl(field_nullability_operand, Immediate(value_cid));
     } else {
       // Value in graph known to be non-null.
-      Label skip_length_check;
       // Compare class id with guard field class id.
       __ cmpl(field_cid_operand, Immediate(value_cid));
-      // If not equal, skip over length check.
-      __ j(NOT_EQUAL, &skip_length_check);
-      // Insert length check.
-      if (field_has_length) {
-        ASSERT(value_cid_reg != kNoRegister);
-        if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
-          __ cmpl(FieldAddress(value_reg, Array::length_offset()),
-                  Immediate(Smi::RawValue(field_length)));
-        } else if (RawObject::IsTypedDataClassId(value_cid)) {
-          __ cmpl(FieldAddress(value_reg, TypedData::length_offset()),
-                  Immediate(Smi::RawValue(field_length)));
-        } else if (field_cid != kIllegalCid) {
-          ASSERT(field_cid != value_cid);
-          ASSERT(field_length >= 0);
-          // Field has a known class id and length. At compile time it is
-          // known that the value's class id is not a fixed length list.
-          __ jmp(fail);
-        } else {
-          ASSERT(field_cid == kIllegalCid);
-          ASSERT(field_length == Field::kUnknownFixedLength);
-          // Following jump cannot not occur, fall through.
-        }
-        __ j(NOT_EQUAL, fail);
-      }
-      // Not identical, possibly null.
-      __ Bind(&skip_length_check);
     }
     // Jump when class id guard and list length guard are okay.

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

remove list length from comment

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

Done.

     __ j(EQUAL, &ok);
 
-    // Check if guard field is uninitialized.
-    __ cmpl(field_cid_operand, Immediate(kIllegalCid));
-    // Jump to failure path when guard field has been initialized and
-    // the field and value class ids do not not match.
-    __ j(NOT_EQUAL, fail);
-
     // At this point the field guard is being initialized for the first time.
-    if (value_cid == kDynamicCid) {
-      // Do not know value's class id.
-      __ movl(field_cid_operand, value_cid_reg);
-      __ movl(field_nullability_operand, value_cid_reg);
-      if (field_has_length) {
-        Label check_array, length_set, no_fixed_length;
-        __ cmpl(value_cid_reg, Immediate(kNullCid));
-        __ j(EQUAL, &no_fixed_length, Assembler::kNearJump);
-        // Check for typed data array.
-        __ cmpl(value_cid_reg, Immediate(kTypedDataInt32x4ArrayCid));
-        // Not a typed array or a regular array.
-        __ j(GREATER, &no_fixed_length, Assembler::kNearJump);
-        __ cmpl(value_cid_reg, Immediate(kTypedDataInt8ArrayCid));
-        // Could still be a regular array.
-        __ j(LESS, &check_array, Assembler::kNearJump);
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ movl(value_cid_reg,
-                FieldAddress(value_reg, TypedData::length_offset()));
-        __ movl(field_length_operand, value_cid_reg);
-        // Updated field length typed data array.
-        __ jmp(&length_set, Assembler::kNearJump);
-        // Check for regular array.
-        __ Bind(&check_array);
-        __ cmpl(value_cid_reg, Immediate(kImmutableArrayCid));
-        __ j(GREATER, &no_fixed_length, Assembler::kNearJump);
-        __ cmpl(value_cid_reg, Immediate(kArrayCid));
-        __ j(LESS, &no_fixed_length, Assembler::kNearJump);
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ movl(value_cid_reg,
-                FieldAddress(value_reg, Array::length_offset()));
-        __ movl(field_length_operand, value_cid_reg);
-        // Updated field length from regular array.
-        __ jmp(&length_set, Assembler::kNearJump);
-        __ Bind(&no_fixed_length);
-        __ movl(field_length_operand,
-                Immediate(Smi::RawValue(Field::kNoFixedLength)));
-        __ Bind(&length_set);
+    if (!field().needs_length_check()) {

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

Maybe add a comment that the GuardFieldLength is expected to occur _after_ the
GuardFieldCid, and they must not be reordered.

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

Done.

+      // Check if guard field is uninitialized.
+      __ cmpl(field_cid_operand, Immediate(kIllegalCid));
+      // Jump to failure path when guard field has been initialized and
+      // the field and value class ids do not not match.
+      __ j(NOT_EQUAL, fail);
+
+      if (value_cid == kDynamicCid) {
+        // Do not know value's class id.
+        __ movl(field_cid_operand, value_cid_reg);
+        __ movl(field_nullability_operand, value_cid_reg);
+      } else {
+        ASSERT(field_reg != kNoRegister);
+        __ movl(field_cid_operand, Immediate(value_cid));
+        __ movl(field_nullability_operand, Immediate(value_cid));
       }
-    } else {
-      ASSERT(field_reg != kNoRegister);
-      __ movl(field_cid_operand, Immediate(value_cid));
-      __ movl(field_nullability_operand, Immediate(value_cid));
-      if (field_has_length) {
-        ASSERT(value_cid_reg != kNoRegister);
-        if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, Array::length_offset()));
-          __ movl(field_length_operand, value_cid_reg);
-        } else if (RawObject::IsTypedDataClassId(value_cid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, TypedData::length_offset()));
-          __ movl(field_length_operand, value_cid_reg);
-        } else {
-          __ movl(field_length_operand,
-                  Immediate(Smi::RawValue(Field::kNoFixedLength)));
-        }
+
+      if (deopt == NULL) {
+        ASSERT(!compiler->is_optimizing());
+        __ jmp(&ok);
       }
     }
 
     if (deopt == NULL) {
       ASSERT(!compiler->is_optimizing());
-      __ jmp(&ok);
       __ Bind(fail);
 
       __ cmpl(FieldAddress(field_reg, Field::guarded_cid_offset()),
@@ -1726,11 +1575,9 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   } else {
     ASSERT(compiler->is_optimizing());
     ASSERT(deopt != NULL);
-    // Field guard class has been initialized and is known.
-    if (field_reg != kNoRegister) {
-      __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
-    }
+    ASSERT(fail == deopt);
 
+    // Field guard class has been initialized and is known.
     if (value_cid == kDynamicCid) {
       // Value's class id is not known.
       __ testl(value_reg, Immediate(kSmiTagMask));
@@ -1741,27 +1588,6 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
         __ cmpl(value_cid_reg, Immediate(field_cid));
       }
 
-      if (field_has_length) {
-        // Jump when Value CID != Field guard CID
-        __ j(NOT_EQUAL, fail);
-
-        // Classes are same, perform guarded list length check.
-        ASSERT(field_reg != kNoRegister);
-        ASSERT(value_cid_reg != kNoRegister);
-        FieldAddress field_length_operand(
-            field_reg, Field::guarded_list_length_offset());
-        if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, Array::length_offset()));
-        } else if (RawObject::IsTypedDataClassId(field_cid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, TypedData::length_offset()));
-        }
-        __ cmpl(value_cid_reg, field_length_operand);
-      }
-
       if (field().is_nullable() && (field_cid != kNullCid)) {
         __ j(EQUAL, &ok);
         const Immediate& raw_null =
@@ -1771,30 +1597,80 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
       __ j(NOT_EQUAL, fail);
     } else {
       // Both value's and field's class id is known.
-      if ((value_cid != field_cid) && (value_cid != nullability)) {
-        __ jmp(fail);
-      } else if (field_has_length && (value_cid == field_cid)) {
-        ASSERT(value_cid_reg != kNoRegister);
-        if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, Array::length_offset()));
-        } else if (RawObject::IsTypedDataClassId(field_cid)) {
-          // Destroy value_cid_reg (safe because we are finished with it).
-          __ movl(value_cid_reg,
-                  FieldAddress(value_reg, TypedData::length_offset()));
-        }
-        __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));
-        __ j(NOT_EQUAL, fail);
-      } else {
-        UNREACHABLE();
-      }
+      ASSERT((value_cid != field_cid) && (value_cid != nullability));
+      __ jmp(fail);
     }
   }
   __ Bind(&ok);
 }
 
 
+LocationSummary* GuardFieldLengthInstr::MakeLocationSummary(Isolate* isolate,
+                                                            bool opt) const {
+  const intptr_t kNumInputs = 1;
+  LocationSummary* summary = new(isolate) LocationSummary(
+      isolate, kNumInputs, 0, LocationSummary::kNoCall);
+  summary->set_in(0, Location::RequiresRegister());
+
+  if (!opt || (field().guarded_list_length() == Field::kUnknownFixedLength)) {
+    // We need temporaries for field object, length offset and expected length.
+    summary->AddTemp(Location::RequiresRegister());
+    summary->AddTemp(Location::RequiresRegister());
+    summary->AddTemp(Location::RequiresRegister());
+  }
+
+  return summary;
+}
+
+
+void GuardFieldLengthInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+  if (field().guarded_list_length() == Field::kNoFixedLength) {

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

ASSERT(!compiler->is_optimizing())?

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

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

Done.

+    return;
+  }
+
+  Register value_reg = locs()->in(0).reg();
+
+  if (!compiler->is_optimizing() ||
+      (field().guarded_list_length() == Field::kUnknownFixedLength)) {
+    Register field_reg = locs()->temp(0).reg();
+    Register offset_reg = locs()->temp(1).reg();
+    Register length_reg = locs()->temp(2).reg();
+
+    Label ok;
+
+    __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
+
+    __ movl(offset_reg, FieldAddress(field_reg,
+        Field::guarded_list_length_in_object_offset_offset()));
+    __ movl(length_reg, FieldAddress(field_reg,
+        Field::guarded_list_length_offset()));
+
+    __ cmpl(offset_reg, Immediate(0));

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

move the compare / jmp above the load of length_reg?

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

Maybe 

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

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

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

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

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.

+    __ j(NEGATIVE, &ok);
+
+    __ cmpl(length_reg, FieldAddress(value_reg, offset_reg, TIMES_1, 0));
+    __ j(EQUAL, &ok);
+
+    __ pushl(field_reg);
+    __ pushl(value_reg);
+    __ CallRuntime(kUpdateFieldCidRuntimeEntry, 2);
+    __ Drop(2);  // Drop the field and the value.
+
+    __ Bind(&ok);
+  } else {
+    Label* deopt = compiler->AddDeoptStub(deopt_id(), ICData::kDeoptGuardField);
+
+    ASSERT(compiler->is_optimizing());
+    ASSERT(field().guarded_list_length() >= 0);
+

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

ASSERT(field().guarded_list_length_in_object_offset() != kUnknownLengthOffset)

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

Done.

+    __ cmpl(FieldAddress(value_reg,
+                         field().guarded_list_length_in_object_offset()),
+            Immediate(Smi::RawValue(field().guarded_list_length())));
+    __ j(NOT_EQUAL, deopt);
+  }
+}
+
+
 class StoreInstanceFieldSlowPath : public SlowPathCode {
  public:
   StoreInstanceFieldSlowPath(StoreInstanceFieldInstr* instruction,