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..4e71fabe39afdeee04e76681984b8e684582ab12 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,15 @@ HObjectAccess HObjectAccess::ForField(Handle<Map> map,
     PropertyDetails details =
         transition->instance_descriptors()->GetDetails(descriptor);
     representation = details.representation();
+    field_type = CONSTANT;
   }
-  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);
-  } else {
-    // Non-negative property indices are in the properties array.
-    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
-    return HObjectAccess(kBackingStore, offset, representation, name);
-  }
+  // Negative property indices are in-object properties, indexed

[titzer, 2013/08/26 13:06:45]

I think I prefer the if; the logic gets too twisted otherwise.

[Toon Verwaest, 2013/08/26 13:39:16]

Ok, I'll revert to it.

On 2013/08/26 13:06:45, titzer wrote:

+  // from the end of the fixed part of the object.
+  // Non-negative property indices are in the properties array.
+  int offset = index * kPointerSize;
+  offset += index < 0 ? map->instance_size() : FixedArray::kHeaderSize;
+  Portion portion = index < 0 ? kInobject : kBackingStore;
+  return HObjectAccess(portion, offset, representation, name, field_type);
 }
 
 
@@ -3927,11 +3928,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);

[titzer, 2013/08/26 13:06:45]

Can a map transition have side effects other than just changing that one word in
the header of that object, e.g. change the layout of an object, thus
invalidating a previously loaded value?

[Toon Verwaest, 2013/08/26 13:39:16]

No. We are transitioning because that property didn't exist yet for that object.
Hence it can't change a value of a property loaded from that object.

Maps do change however, so DeopendsOnMaps values can't be hoisted over it.
HCheckMaps eg does depend on maps. And HLoadNamedField depend on HCheckMaps.

For some reason kChangesMaps is still set manually in BuildStoreNamedField.

On 2013/08/26 13:06:45, titzer wrote:

   }
 
+  if (FieldTypeField::decode(value_) == CONSTANT) return;
+
   switch (portion()) {
     case kArrayLengths:
       instr->SetGVNFlag(is_store