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

src/hydrogen-instructions.cc

Index: src/hydrogen-instructions.cc
diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index fc57aa879c68111e4158f7db07ebe685c2790110..37f7d425b786fc7233b6ba1323ea74c21314f372 100644
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -2464,7 +2464,7 @@ void HParameter::PrintDataTo(StringStream* stream) {
 
 void HLoadNamedField::PrintDataTo(StringStream* stream) {
   object()->PrintNameTo(stream);
-  stream->Add(" @%d%s", offset(), is_in_object() ? "[in-object]" : "");
+  access_.PrintTo(stream);
   if (HasTypeCheck()) {
     stream->Add(" ");
     typecheck()->PrintNameTo(stream);
@@ -2757,11 +2757,9 @@ void HStoreNamedGeneric::PrintDataTo(StringStream* stream) {
 
 void HStoreNamedField::PrintDataTo(StringStream* stream) {
   object()->PrintNameTo(stream);
-  stream->Add(".");
-  stream->Add(*String::cast(*name())->ToCString());
+  access_.PrintTo(stream);
   stream->Add(" = ");
   value()->PrintNameTo(stream);
-  stream->Add(" @%d%s", offset(), is_in_object() ? "[in-object]" : "");
   if (NeedsWriteBarrier()) {
     stream->Add(" (write-barrier)");
   }
@@ -3628,4 +3626,139 @@ void HCheckFunction::Verify() {
 
 #endif
 
+
+HObjectAccess HObjectAccess::ForFixedArrayOffset(int offset) {
+  ASSERT(offset >= 0);
+  if (offset == FixedArray::kLengthOffset) return ForFixedArrayLength();
+  return HObjectAccess(kInobject, offset);

[danno, 2013/05/10 15:59:19]

How about handling the map at offset zero like others? For now offsets >
FixedArray::kLengthOffset should assert, since they need to be KeyedLoads.

[titzer, 2013/05/13 11:23:19]

Unfortunately we use this variant to construct a direct access to an element of
a fixed array in BuildNativeGetContext :(

[danno, 2013/05/13 15:44:32]

OK, then you will have to be very careful here. There are two types of
FixedArrays, FixedArrays and FixedDouble arrays. They both share a common base
class, but depending on which sub-class you use of FixedArray base, you need to
not invalidate kInobject but rather ArrayElements or DoubleArrayElements.

I think this should actually be FixedArrayBaseOffset that only handles the
header, and you should change the context-accessing code to use a KeyedLoad,
which handled the dependencies correctly depending on the elements kind, IIRC
(in this case, using a NamedLoad to access the context probably works but is not
right in principle). You can just pass a HConstant as the right index and it
will be just as efficient as the HNamedLoad

On 2013/05/13 11:23:19, titzer wrote:

[titzer, 2013/05/14 09:26:19]

I've renamed this to ForFixedArrayHeader, and only allow this method to be used
with offsets within the header, replacing the other uses with HKeyedLoad as
suggested.

+}
+
+
+HObjectAccess HObjectAccess::ForJSObjectOffset(int offset,
+    Handle<String> name) {
+  ASSERT(offset >= 0);

[danno, 2013/05/10 15:59:19]

I think this assert is bogus (maps?)

[titzer, 2013/05/13 11:23:19]

Maps seem to be at offset 0 on ia32. Is this true for all platforms?

[danno, 2013/05/13 15:44:32]

Ah, yes, it's not -1, it's still untagged at this point.
On 2013/05/13 11:23:19, titzer wrote:

+  Portion portion = kInobject;
+
+  if (offset == JSObject::kElementsOffset) {
+    portion = kElementsPointer;
+  } else if (offset == JSObject::kMapOffset) {
+    portion = kMaps;
+  }
+  return HObjectAccess(portion, offset, name);
+}
+
+
+HObjectAccess HObjectAccess::ForJSArrayOffset(int offset,
+    Handle<String> name) {
+  ASSERT(offset >= 0);

[danno, 2013/05/10 15:59:19]

I think this assert is bogus (maps?)

[danno, 2013/05/13 15:44:32]

Yeah, ignore this
On 2013/05/10 15:59:19, danno wrote:

+  Portion portion = kInobject;
+
+  if (offset == JSObject::kElementsOffset) {
+    portion = kElementsPointer;
+  } else if (offset == JSArray::kLengthOffset) {
+    portion = kArrayLengths;
+  } else if (offset == JSObject::kMapOffset) {
+    portion = kMaps;
+  }
+  return HObjectAccess(portion, offset, name);
+}
+
+
+HObjectAccess HObjectAccess::ForBackingStoreOffset(int offset,
+    Handle<String> name) {
+  ASSERT(offset >= 0);
+  return HObjectAccess(kBackingStore, offset, name);
+}
+
+
+
+HObjectAccess HObjectAccess::ForField(Handle<Map> map,
+    LookupResult *lookup, Handle<String> name) {
+  ASSERT(lookup->IsField() || lookup->IsTransitionToField(*map));
+  int index;
+  if (lookup->IsField()) {
+    index = lookup->GetLocalFieldIndexFromMap(*map);
+  } else {
+    Map* transition = lookup->GetTransitionMapFromMap(*map);
+    int descriptor = transition->LastAdded();
+    index = transition->instance_descriptors()->GetFieldIndex(descriptor) -
+        map->inobject_properties();
+  }
+  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);
+  } else {
+    // Non-negative property indices are in the properties array.
+    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
+    return HObjectAccess(kBackingStore, offset, name);
+  }
+}
+
+
+void HObjectAccess::SetGVNFlags(HValue *instr, bool is_store) {
+  // set the appropriate GVN flags for a given load or store instruction
+  if (is_store) {
+    // track dominating allocations in order to eliminate write barriers
+    instr->SetGVNFlag(kDependsOnNewSpacePromotion);
+    instr->SetFlag(HValue::kTrackSideEffectDominators);
+  } else {
+    // try to GVN loads, but don't hoist have map changes
+    instr->SetFlag(HValue::kUseGVN);
+    instr->SetGVNFlag(kDependsOnMaps);
+  }
+
+  switch (portion_) {
+    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;
+  }
+}
+
+
+void HObjectAccess::PrintTo(StringStream* stream) {
+  stream->Add(".@%d", offset_);

[danno, 2013/05/13 15:44:32]

I'd prefer the output of the form (it's the way it was before and it was more
JavaScript-esque):

o.field_name@2345

field_name should use "%"-prefixed field names for private (internal) field as
they were defined before your CL, e.g. (%map, %length,etc).

[titzer, 2013/05/14 09:26:19]

Done.

+
+  // several portions have well-known names
+  switch (portion_) {
+    case kArrayLengths:
+      stream->Add("[array-length]");
+      return;
+    case kElementsPointer:
+      stream->Add("[elements]");
+      return;
+    case kMaps:
+      stream->Add("[map]");
+      return;
+    case kInobject:
+      stream->Add("[in-object]");

[danno, 2013/05/13 15:44:32]

Shouldn't you only print this if there is no name, i.e. when name.is_null:

o.<unknown-in-object> 

[titzer, 2013/05/14 09:26:19]

I believe the old code would print [in-object] or [backing-store] in any case. I
think this may be useful in the hydrogen output as well.

+      break;  // also print the name if possible
+  }
+
+  // otherwise, add the name if it is known
+  if (!name_.is_null()) {

[danno, 2013/05/13 15:44:32]

Seems like this should be mutually exclusive with everything except kInobject.
An assert to make sure?

[titzer, 2013/05/14 09:26:19]

Currently, yes, but I'd rather not put an assert in for something harmless.
Having an extra name in an access might be useful in the future, e.g. to
designate "payload" and other special fields.

+    stream->Add(" name[");
+    stream->Add(*String::cast(*name_)->ToCString());

[danno, 2013/05/13 15:44:32]

See above, please use existing JavaScript-like syntax for printing field names.

[titzer, 2013/05/14 09:26:19]

Done.

+    stream->Add("]");
+  }
+}
+
 } }  // namespace v8::internal