Introduce HObjectAccess, which is used by LoadNamedField and StoreNamedField to denote what parts

src/hydrogen-instructions.h

Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 0c3ee1dc081101e47646daa14e0c1beb464f9ceb..772185a7446054a3ea9f54382286f19e4d75eb87 100644
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -5238,35 +5238,92 @@ class HStoreContextSlot: public HTemplateInstruction<2> {
 };
 
 
+// Represents an access (load or store) to a portion of an object for GVN.
+// TODO(titzer): only represents non-array-element loads and stores for now
+class ObjectAccess {
+ public:
+  enum Portion {
+    kElementsKind,

[danno, 2013/04/26 09:39:38]

The seems redundant to me. It seems like this is what the flags represent in the
first place. Instead, I suggest SetGVNFlags use the map and offset and a little
more complicated logic to determine the right flags. This should do the trick:

if (map.is_null) {
  // Might want to change "SpecializedArrayElements" to "NonHeapMemory"
  instr->SetGVNFlag(is_store ? kChangesSpecializedArrayElements :
kDependsOnSpecializedArrayElements);
  return;
} else if (offset == 0) {
  instr->SetGVNFlag(is_store ? kChangesMaps : kDependsOnMaps);
  return;
} else if (map == heap->fixed_array_map() ||
             map == heap->fixed_double_array_map()) {
  if (offset == FixedArray::kLengthOffset) {
    instr->SetGVNFlag(is_store ? kChangesArrayLengths : kDependsOnArrayLengths);
  } else {
     if (map == heap->fixed_array_map()) {
       instr->SetGVNFlag(is_store ? kChangesArrayElements :
kDependsOnArrayElements);
     } else {
       instr->SetGVNFlag(is_store ? kChangesDoubleArrayElements :
kDependsOnDoubleArrayElements);
     }
  }
  return;
}

// Must be a JSObject or a JSArray
ASSERT(map->instance_type() == JS_OBJECT ||
            map->instance_type() == JS_ARRAY);

if (offset == JSObject::kPropertiesOffset) {
   instr->SetGVNFlag(is_store ? kChangesPropertyPointer :
kDependsOnPropertyPointer)
   return;
} else if (offset == JSObject::kElementsOffset) {
   instr->SetGVNFlag(is_store ? kChangesElementsPointer :
kDependsOnElementsPointer)
   return;
}

if (map->instance_type() == JS_ARRAY) {
  ASSERT(offset == JSArray::kArrayLength);
  instr->SetGVNFlag(is_store ? kChangesArrayLengths : kDependsOnArrayLengths)
  return;

if (in_object) {
  instr->SetGVNFlag(is_store ? kChangesInobjectFields :
kDependsOnInobjectFields)
} else {
  instr->SetGVNFlag(is_store ? kChangesBackingStoreFields :
kDependsOnBackingStoreFields)
}

[titzer, 2013/04/30 15:56:47]

As discussed in person, I think we can make this work in a follow-up CL where we
move away from having an enum specifying different object portions. For this
intermediate point, the enum essentially preserves what is known at the object
access construction sites, rather than relying on a method like the above to
reconstruct it.

+    kArrayLengths,
+    kInobject,
+    kBackingStore,
+    kElementsPointer,
+    kMaps
+  };
+
+  ObjectAccess(Portion portion, int offset, bool may_allocate,
+    Handle<String> name = Handle<String>::null())
+    : portion_(portion), offset_(offset), may_allocate_(may_allocate) { }
+
+  void SetGVNFlags(HValue *instr, bool is_store) {

[danno, 2013/04/26 09:39:38]

Make this protected and add HLoadNamedFiled and HStoreNamedField friends.

[titzer, 2013/04/30 15:56:47]

Done.

+    // set the appropriate GVN flags for a given load or store instruction
+    if (!is_store) {
+      instr->SetFlag(HValue::kUseGVN);    // try to GVN loads
+      instr->SetGVNFlag(kDependsOnMaps);  // but don't hoist above map changes
+    }
+
+    // if this instruction might trigger an allocation
+    if (may_allocate_) instr->SetGVNFlag(kChangesNewSpacePromotion);

[danno, 2013/04/26 09:39:38]

When does this actually happen? We should try to get rid of it, loads/stores
shouldn't allocate themselves.

[titzer, 2013/04/30 15:56:47]

Done.

+
+    switch (portion_) {
+    case kElementsKind:
+      instr->SetGVNFlag(
+          is_store ? kChangesElementsKind : kDependsOnElementsKind);

[danno, 2013/04/26 09:39:38]

Don't know if it makes the code cleaner, but maybe you can make use of
ConvertChangesToDependsFlags here somehow for the store case in one place?

[titzer, 2013/04/30 15:56:47]

That method takes a GVNFlagSet and does a shift. I toyed with having a version
that converts a single changes flag to a depends flag, but it is essentially a
mysterious +1 and didn't save enough here to be worth it IMO.

+      break;
+    case kArrayLengths:
+      instr->SetGVNFlag(
+          is_store ? kChangesArrayLengths : kDependsOnArrayLengths);
+      break;
+    case kInobject:
+      instr->SetGVNFlag(
+          is_store ? kChangesInobjectFields : kDependsOnInobjectFields);
+      break;
+    case kBackingStore:
+      instr->SetGVNFlag(
+          is_store ? kChangesBackingStoreFields : kDependsOnBackingStoreFields);
+      break;
+    case kElementsPointer:
+      instr->SetGVNFlag(
+          is_store ? kChangesElementsPointer : kDependsOnElementsPointer);
+      break;
+    case kMaps:
+      instr->SetGVNFlag(
+          is_store ? kChangesMaps : kDependsOnMaps);
+      break;
+    }

[danno, 2013/04/26 09:39:38]

This guy really belongs in the .cc file.

[titzer, 2013/04/30 15:56:47]

Done.

+  }
+
+  inline bool IsInobject() {
+    return portion_ != kBackingStore;
+  }
+
+  inline int offset() {
+    return offset_;
+  }
+
+  inline Handle<String> name() {

[danno, 2013/04/26 09:39:38]

This can be calculated by looking in the map's field descriptors for the right
offset.

[titzer, 2013/04/30 15:56:47]

I agree with that approach in the long run. We can do that when ObjectAccesses
consists of a map and an offset.

+    return name_;
+  }
+
+ private:
+  Portion portion_;
+  int offset_;
+  bool may_allocate_;
+  Handle<String> name_;

[danno, 2013/04/26 09:39:38]

You don't need to store this it can be looked-up if necessary. Part of your
change should remove the hard requirement on stores for the field name (or maybe
a precursor CL).

I think the only values you should have to store are:

int offset_: 30
bool in_object_ : 1
Handle<Map> map_;

If you just have these fields, you can just embed a copy of the ObjectAccess
into the HLoadNamedField/HStoreField instruction itself.

[danno, 2013/05/02 14:16:47]

You didn't address this comment. Why not?

On 2013/04/26 09:39:38, danno wrote:

+};
+
+
 class HLoadNamedField: public HTemplateInstruction<2> {
  public:
-  HLoadNamedField(HValue* object, bool is_in_object, int offset,
-                  HValue* typecheck = NULL)
-      : is_in_object_(is_in_object),
-        offset_(offset) {
+  HLoadNamedField(HValue* object, ObjectAccess access, HValue* typecheck = NULL)

[danno, 2013/04/26 09:39:38]

Unless the objects is the size of intptr_t, then we always pass by pointer (not
by reference) and do not copy.

[titzer, 2013/04/30 15:56:47]

Done.

+      : is_in_object_(access.IsInobject()),
+        offset_(access.offset()) {
     ASSERT(object != NULL);
     SetOperandAt(0, object);
     SetOperandAt(1, typecheck != NULL ? typecheck : object);
 
     set_representation(Representation::Tagged());
-    SetFlag(kUseGVN);
-    SetGVNFlag(kDependsOnMaps);
-    if (is_in_object) {
-      SetGVNFlag(kDependsOnInobjectFields);
-    } else {
-      SetGVNFlag(kDependsOnBackingStoreFields);
-    }
-  }
-
-  static HLoadNamedField* NewArrayLength(Zone* zone, HValue* object,
-                                         HValue* typecheck,
-                                         HType type = HType::Tagged()) {
-    HLoadNamedField* result = new(zone) HLoadNamedField(
-        object, true, JSArray::kLengthOffset, typecheck);
-    result->set_type(type);
-    result->SetGVNFlag(kDependsOnArrayLengths);
-    result->ClearGVNFlag(kDependsOnInobjectFields);
-    return result;
+    access.SetGVNFlags(this, false);
   }
 
   HValue* object() { return OperandAt(0); }
@@ -5590,24 +5647,17 @@ class HLoadKeyedGeneric: public HTemplateInstruction<3> {
 class HStoreNamedField: public HTemplateInstruction<2> {
  public:
   HStoreNamedField(HValue* obj,
-                   Handle<String> name,
-                   HValue* val,
-                   bool in_object,
-                   int offset)
-      : name_(name),
-        is_in_object_(in_object),
-        offset_(offset),
+                   ObjectAccess access,
+                   HValue* val)
+      : name_(access.name()),
+        is_in_object_(access.IsInobject()),
+        offset_(access.offset()),
         transition_unique_id_(),
         new_space_dominator_(NULL) {
     SetOperandAt(0, obj);
     SetOperandAt(1, val);
     SetFlag(kTrackSideEffectDominators);
-    SetGVNFlag(kDependsOnNewSpacePromotion);
-    if (is_in_object_) {
-      SetGVNFlag(kChangesInobjectFields);
-    } else {
-      SetGVNFlag(kChangesBackingStoreFields);
-    }
+    access.SetGVNFlags(this, true);
   }
 
   DECLARE_CONCRETE_INSTRUCTION(StoreNamedField)