Avoid setting / depending on flags when transitioning, and when fields are known to be constant.

src/hydrogen-instructions.cc

Index: src/hydrogen-instructions.cc
diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index b0045b8751b06258f0a241416e54615924ea910f..d745e5164664c9894c49492854981183f7ee2597 100644
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -3884,13 +3884,16 @@ HObjectAccess HObjectAccess::ForBackingStoreOffset(int offset,
 
 
 HObjectAccess HObjectAccess::ForField(Handle<Map> map,
-    LookupResult *lookup, Handle<String> name) {
+                                      LookupResult *lookup,
+                                      Handle<String> name) {
   ASSERT(lookup->IsField() || lookup->IsTransitionToField(*map));
   int index;
   Representation representation;
+  FieldType field_type;
   if (lookup->IsField()) {
     index = lookup->GetLocalFieldIndexFromMap(*map);
     representation = lookup->representation();
+    field_type = MUTABLE;
   } else {
     Map* transition = lookup->GetTransitionMapFromMap(*map);
     int descriptor = transition->LastAdded();
@@ -3899,17 +3902,21 @@ HObjectAccess HObjectAccess::ForField(Handle<Map> map,
     PropertyDetails details =
         transition->instance_descriptors()->GetDetails(descriptor);
     representation = details.representation();
+    field_type = CONSTANT;
   }
+  int offset = index * kPointerSize;
+  Portion portion;
   if (index < 0) {
-    // Negative property indices are in-object properties, indexed
-    // from the end of the fixed part of the object.
-    int offset = (index * kPointerSize) + map->instance_size();
-    return HObjectAccess(kInobject, offset, representation);
+    // Negative property indices are in-object properties, indexed from the end
+    // of the fixed part of the object.
+    offset += map->instance_size();
+    portion = kInobject;
   } else {
     // Non-negative property indices are in the properties array.
-    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
-    return HObjectAccess(kBackingStore, offset, representation, name);
+    offset += FixedArray::kHeaderSize;
+    portion = kBackingStore;
   }
+  return HObjectAccess(portion, offset, representation, name, field_type);
 }
 
 
@@ -3927,11 +3934,11 @@ void HObjectAccess::SetGVNFlags(HValue *instr, bool is_store) {
     instr->SetGVNFlag(kDependsOnNewSpacePromotion);
     instr->SetFlag(HValue::kTrackSideEffectDominators);
   } else {
-    // try to GVN loads, but don't hoist above map changes
     instr->SetFlag(HValue::kUseGVN);
-    instr->SetGVNFlag(kDependsOnMaps);
   }
 
+  if (FieldTypeField::decode(value_) == CONSTANT) return;

[titzer, 2013/08/26 15:33:47]

Mmmm, this is starting to make me feel uncomfortable.

Here is the problem. With a CONSTANT load, it no longer depends on _any_ GVN
flags. But you can also have a CONSTANT store, which doesn't change _any_ GVN
flags (but does depend on NewSpacePromotion--which is somewhat weird). The
problem is that once those flags are gone, all bets are off. The load could get
lifted above the store, and then it will get the uninitialized value, not the
stored value.

We are likely to get lucky just because GVN and LICM always lift instructions in
their original order. That is not always going to be the case.

We should express the load/store dependency here with an explicit edge--i.e. the
load depends directly on the store.

In the future we will have load/store elimination, but even then, we need to
make this dependency explicit.

+
   switch (portion()) {
     case kArrayLengths:
       instr->SetGVNFlag(is_store