Fix polymorphic to monomorphic load to take representation into account.

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 58a9b78750bb7401bfdc9e167800f8f01d3ef078..637f1d03df17bc7fe59241c68e5b7eacd746d29c 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -7131,22 +7131,30 @@ void HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
     HValue* object,
     SmallMapList* types,
     Handle<String> name) {
-  int count = 0;
-  int previous_field_offset = 0;
-  bool previous_field_is_in_object = false;
-  bool is_monomorphic_field = true;
 
   if (HandlePolymorphicArrayLengthLoad(expr, object, types, name))
     return;
 
-  Handle<Map> map;
-  LookupResult lookup(isolate());
-  for (int i = 0; i < types->length() && count < kMaxLoadPolymorphism; ++i) {
-    map = types->at(i);
-    if (ComputeLoadStoreField(map, name, &lookup, false)) {
+  AddInstruction(new(zone()) HCheckNonSmi(object));
+
+  // Use monomorphic load if property lookup results in the same field index
+  // for all maps. Requires special map check on the set of all handled maps.
+  if (types->length() > 0 && types->length() <= kMaxLoadPolymorphism) {

[mvstanton, 2013/05/07 10:17:17]

I like this if check against kMaxLoadPolymorphism, it is clearer than before.

[Toon Verwaest, 2013/05/07 10:30:24]

Done.

+    LookupResult lookup(isolate());
+    int previous_field_offset = 0;
+    bool previous_field_is_in_object = false;
+    Representation representation = Representation::None();
+    int count;
+    for (count = 0; count < types->length(); ++count) {
+      Handle<Map> map = types->at(count);
+      if (!ComputeLoadStoreField(map, name, &lookup, false)) break;
+
       int index = ComputeLoadStoreFieldIndex(map, &lookup);
+      Representation new_representation =
+          ComputeLoadStoreRepresentation(map, &lookup);
       bool is_in_object = index < 0;
       int offset = index * kPointerSize;
+
       if (index < 0) {
         // Negative property indices are in-object properties, indexed
         // from the end of the fixed part of the object.
@@ -7154,33 +7162,34 @@ void HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
       } else {
         offset += FixedArray::kHeaderSize;
       }
+
       if (count == 0) {
         previous_field_offset = offset;
         previous_field_is_in_object = is_in_object;
-      } else if (is_monomorphic_field) {
-        is_monomorphic_field = (offset == previous_field_offset) &&
-                               (is_in_object == previous_field_is_in_object);
+        representation = new_representation;
+      } else if (offset != previous_field_offset ||
+                 is_in_object != previous_field_is_in_object ||
+                 (FLAG_track_fields &&
+                  !representation.IsCompatibleForLoad(new_representation))) {
+        break;
       }
-      ++count;
+
+      representation = representation.generalize(new_representation);
     }
-  }
 
-  // Use monomorphic load if property lookup results in the same field index
-  // for all maps.  Requires special map check on the set of all handled maps.
-  AddInstruction(new(zone()) HCheckNonSmi(object));
-  HInstruction* instr;
-  if (count == types->length() && is_monomorphic_field) {
-    AddInstruction(HCheckMaps::New(object, types, zone()));
-    instr = BuildLoadNamedField(object, map, &lookup);
-  } else {
-    HValue* context = environment()->LookupContext();
-    instr = new(zone()) HLoadNamedFieldPolymorphic(context,
-                                                   object,
-                                                   types,
-                                                   name,
-                                                   zone());
+    if (count == types->length()) {
+      AddInstruction(HCheckMaps::New(object, types, zone()));
+      HInstruction* instr = DoBuildLoadNamedField(
+          object, previous_field_is_in_object,
+          representation, previous_field_offset);
+      instr->set_position(expr->position());

[mvstanton, 2013/05/07 10:17:17]

the instr->set_position() line and ast_context() line are unfortunate in that
they don't seem to have anything to do with the rest of the function. That is
okay, except that they are duplicated. Is there a way to just run those lines
once for the instruction created, whether it is an HLoadNamedFieldPolymorphic()
or a DoBuildLoadNamedField()?

Maybe you have instr = NULL; before the loop case, and then check that value
below the loop. That way you only have to write the lines I'm complaining about
once at the end of the function. You also still got to eliminate carrying all
the previous_field_offset crap out of your loop, since you build the instruction
in the loop.

[Toon Verwaest, 2013/05/07 10:30:24]

Done.

+      return ast_context()->ReturnInstruction(instr, expr->id());
+    }
   }
 
+  HValue* context = environment()->LookupContext();
+  HInstruction* instr = new(zone()) HLoadNamedFieldPolymorphic(
+      context, object, types, name, zone());
   instr->set_position(expr->position());
   return ast_context()->ReturnInstruction(instr, expr->id());
 }
@@ -7735,18 +7744,25 @@ HLoadNamedField* HOptimizedGraphBuilder::BuildLoadNamedField(
     HValue* object,
     Handle<Map> map,
     LookupResult* lookup) {
-  Representation representation = lookup->representation();
   int index = lookup->GetLocalFieldIndexFromMap(*map);
-  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 new(zone()) HLoadNamedField(object, true, representation, offset);
-  } else {
-    // Non-negative property indices are in the properties array.
-    int offset = (index * kPointerSize) + FixedArray::kHeaderSize;
-    return new(zone()) HLoadNamedField(object, false, representation, offset);
-  }
+  // Negative property indices are in-object properties, indexed from the end of
+  // the fixed part of the object. Non-negative property indices are in the
+  // properties array.
+  int inobject = index < 0;
+  Representation representation = lookup->representation();
+  int offset = inobject
+      ? index * kPointerSize + map->instance_size()
+      : index * kPointerSize + FixedArray::kHeaderSize;
+  return DoBuildLoadNamedField(object, inobject, representation, offset);
+}
+
+
+HLoadNamedField* HGraphBuilder::DoBuildLoadNamedField(
+    HValue* object,
+    bool inobject,
+    Representation representation,
+    int offset) {
+  return new(zone()) HLoadNamedField(object, inobject, representation, offset);
 }