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

src/hydrogen-instructions.cc

 // 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 2446 matching lines @@
 }
 
 
 void HParameter::PrintDataTo(StringStream* stream) {
   stream->Add("%u", index());
 }
 
 
 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);
   }
 }
 
 
 // Returns true if an instance of this map can never find a property with this
 // name in its prototype chain.  This means all prototypes up to the top are
 // fast and don't have the name in them.  It would be good if we could optimize
@@ skipping 272 matching lines @@
   stream->Add(".");
   ASSERT(name()->IsString());
   stream->Add(*String::cast(*name())->ToCString());
   stream->Add(" = ");
   value()->PrintNameTo(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)");
   }
   if (!transition().is_null()) {
     stream->Add(" (transition map %p)", *transition());
   }
 }
 
 
 void HStoreKeyed::PrintDataTo(StringStream* stream) {
@@ skipping 846 matching lines @@
 }
 
 
 void HCheckFunction::Verify() {
   HInstruction::Verify();
   ASSERT(HasNoUses());
 }
 
 #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