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

src/hydrogen-instructions.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 5220 matching lines @@
   virtual void PrintDataTo(StringStream* stream);
 
   DECLARE_CONCRETE_INSTRUCTION(StoreContextSlot)
 
  private:
   int slot_index_;
   Mode mode_;
 };
 
 
+// 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); }
   HValue* typecheck() {
     ASSERT(HasTypeCheck());
     return OperandAt(1);
   }
 
   bool HasTypeCheck() const { return OperandAt(0) != OperandAt(1); }
   bool is_in_object() const { return is_in_object_; }
@@ skipping 303 matching lines @@
 
   virtual HValue* Canonicalize();
 
   DECLARE_CONCRETE_INSTRUCTION(LoadKeyedGeneric)
 };
 
 
 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)
 
   virtual Representation RequiredInputRepresentation(int index) {
     return Representation::Tagged();
   }
   virtual void SetSideEffectDominator(GVNFlag side_effect, HValue* dominator) {
     ASSERT(side_effect == kChangesNewSpacePromotion);
     new_space_dominator_ = dominator;
@@ skipping 929 matching lines @@
   virtual bool IsDeletable() const { return true; }
 };
 
 
 #undef DECLARE_INSTRUCTION
 #undef DECLARE_CONCRETE_INSTRUCTION
 
 } }  // namespace v8::internal
 
 #endif  // V8_HYDROGEN_INSTRUCTIONS_H_