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;
   }
-  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);
-  } else {
-    // Non-negative property indices are in the properties array.
-    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
-    return HObjectAccess(kBackingStore, offset, representation, name);
-  }
+  // Negative property indices are in-object properties, indexed

[titzer, 2013/08/26 13:06:45]

I think I prefer the if; the logic gets too twisted otherwise.

[Toon Verwaest, 2013/08/26 13:39:16]

Ok, I'll revert to it.

On 2013/08/26 13:06:45, titzer wrote:

+  // from the end of the fixed part of the object.
+  // Non-negative property indices are in the properties array.
+  int offset = index * kPointerSize;
+  offset += index < 0 ? map->instance_size() : FixedArray::kHeaderSize;
+  Portion portion = index < 0 ? kInobject : 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);

[titzer, 2013/08/26 13:06:45]

Can a map transition have side effects other than just changing that one word in
the header of that object, e.g. change the layout of an object, thus
invalidating a previously loaded value?

[Toon Verwaest, 2013/08/26 13:39:16]

No. We are transitioning because that property didn't exist yet for that object.
Hence it can't change a value of a property loaded from that object.

Maps do change however, so DeopendsOnMaps values can't be hoisted over it.
HCheckMaps eg does depend on maps. And HLoadNamedField depend on HCheckMaps.

For some reason kChangesMaps is still set manually in BuildStoreNamedField.

On 2013/08/26 13:06:45, titzer wrote:

   }
 
+  if (FieldTypeField::decode(value_) == CONSTANT) return;
+
   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