Avoid setting / depending on flags when transitioning, and when fields are known to be constant.

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 3866 matching lines @@
 
 
 HObjectAccess HObjectAccess::ForBackingStoreOffset(int offset,
     Representation representation) {
   ASSERT(offset >= 0);
   return HObjectAccess(kBackingStore, offset, representation);
 }
 
 
 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();
     index = transition->instance_descriptors()->GetFieldIndex(descriptor) -
         map->inobject_properties();
     PropertyDetails details =
         transition->instance_descriptors()->GetDetails(descriptor);
     representation = details.representation();
+    field_type = CONSTANT;
   }
+  int offset = index * kPointerSize;
+  Portion portion;
   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);
+    // Negative property indices are in-object properties, indexed from the end
+    // of the fixed part of the object.
+    offset += map->instance_size();
+    portion = kInobject;
   } else {
     // Non-negative property indices are in the properties array.
-    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
-    return HObjectAccess(kBackingStore, offset, representation, name);
+    offset += FixedArray::kHeaderSize;
+    portion = kBackingStore;
   }
+  return HObjectAccess(portion, offset, representation, name, field_type);
 }
 
 
 HObjectAccess HObjectAccess::ForCellPayload(Isolate* isolate) {
   return HObjectAccess(
       kInobject, Cell::kValueOffset, Representation::Tagged(),
       Handle<String>(isolate->heap()->cell_value_string()));
 }
 
 
 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 above map changes
     instr->SetFlag(HValue::kUseGVN);
-    instr->SetGVNFlag(kDependsOnMaps);
   }
 
+  if (FieldTypeField::decode(value_) == CONSTANT) return;

[titzer, 2013/08/26 15:33:47]

Mmmm, this is starting to make me feel uncomfortable.

Here is the problem. With a CONSTANT load, it no longer depends on _any_ GVN
flags. But you can also have a CONSTANT store, which doesn't change _any_ GVN
flags (but does depend on NewSpacePromotion--which is somewhat weird). The
problem is that once those flags are gone, all bets are off. The load could get
lifted above the store, and then it will get the uninitialized value, not the
stored value.

We are likely to get lucky just because GVN and LICM always lift instructions in
their original order. That is not always going to be the case.

We should express the load/store dependency here with an explicit edge--i.e. the
load depends directly on the store.

In the future we will have load/store elimination, but even then, we need to
make this dependency explicit.

+
   switch (portion()) {
     case kArrayLengths:
       instr->SetGVNFlag(is_store
           ? kChangesArrayLengths : kDependsOnArrayLengths);
       break;
     case kStringLengths:
       instr->SetGVNFlag(is_store
           ? kChangesStringLengths : kDependsOnStringLengths);
       break;
     case kInobject:
@@ skipping 49 matching lines @@
       break;
     case kExternalMemory:
       stream->Add("[external-memory]");
       break;
   }
 
   stream->Add("@%d", offset());
 }
 
 } }  // namespace v8::internal