Refactor turning polymorphic loads into monomorphic loads

src/hydrogen.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 6298 matching lines @@
   if (ComputeLoadStoreField(map, name, &lookup, true)) {
     AddCheckMapsWithTransitions(object, map);
     return BuildStoreNamedField(object, name, value, map, &lookup);
   }
 
   // No luck, do a generic store.
   return BuildStoreNamedGeneric(object, name, value);
 }
 
 
-bool HOptimizedGraphBuilder::HandlePolymorphicArrayLengthLoad(
+HInstruction* HOptimizedGraphBuilder::LoadPolymorphicAsMonomorphic(

[Hannes Payer (out of office), 2013/06/13 15:26:36]

Would it make more sense to name this method TryLoadPolymorphicAsMonomorphic?

[Toon Verwaest, 2013/06/13 16:33:58]

Done.

     Property* expr,
     HValue* object,
     SmallMapList* types,
     Handle<String> name) {
-  if (!name->Equals(isolate()->heap()->length_string())) return false;
-
-  for (int i = 0; i < types->length(); i++) {
-    if (types->at(i)->instance_type() != JS_ARRAY_TYPE) return false;
-  }
-
-  BuildCheckNonSmi(object);
-
-  HInstruction* typecheck =
-      AddInstruction(HCheckMaps::New(object, types, zone()));
-  HInstruction* instr = new(zone())
-      HLoadNamedField(object, HObjectAccess::ForArrayLength(), typecheck);
-
-  instr->set_position(expr->position());
-  ast_context()->ReturnInstruction(instr, expr->id());
-  return true;
-}
-
-
-void HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
-    HValue* object,
-    SmallMapList* types,
-    Handle<String> name) {
-
-  if (HandlePolymorphicArrayLengthLoad(expr, object, types, name))
-    return;
-
-  BuildCheckNonSmi(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.
-  HInstruction* instr = NULL;
   LookupResult lookup(isolate());
   int count;
   Representation representation = Representation::None();
   HObjectAccess access = HObjectAccess::ForMap();  // initial value unused.
   for (count = 0;
        count < types->length() && count < kMaxLoadPolymorphism;
        ++count) {
     Handle<Map> map = types->at(count);
     if (!ComputeLoadStoreField(map, name, &lookup, false)) break;
 
@@ skipping 10 matching lines @@
       break;
     } else if (access.offset() != new_access.offset()) {
       // Offsets did not match.
       break;
     } else if (access.IsInobject() != new_access.IsInobject()) {
       // In-objectness did not match.
       break;
     }
   }
 
-  if (count == types->length()) {
-    // Everything matched; can use monomorphic load.
-    AddInstruction(HCheckMaps::New(object, types, zone()));
-    instr = BuildLoadNamedField(object, access, representation);
-  } else {
+  if (count != types->length()) return NULL;
+
+  // Everything matched; can use monomorphic load.
+  BuildCheckNonSmi(object);
+  AddInstruction(HCheckMaps::New(object, types, zone()));
+  return BuildLoadNamedField(object, access, representation);
+}
+
+
+void HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,

[Hannes Payer (out of office), 2013/06/13 15:26:36]

Can you move "Property* expr" to the next line?

[Toon Verwaest, 2013/06/13 16:33:58]

Done.

+    HValue* object,
+    SmallMapList* types,
+    Handle<String> name) {
+

[Hannes Payer (out of office), 2013/06/13 15:26:36]

remove this newline

[Toon Verwaest, 2013/06/13 16:33:58]

Done.

+  HInstruction* instr = LoadPolymorphicAsMonomorphic(expr, object, types, name);
+  if (instr == NULL) {
     // Something did not match; must use a polymorphic load.
+    BuildCheckNonSmi(object);
     HValue* context = environment()->LookupContext();
     instr = new(zone()) HLoadNamedFieldPolymorphic(
         context, object, types, name, zone());
   }
 
   instr->set_position(expr->position());
   return ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
@@ skipping 274 matching lines @@
     return ast_context()->ReturnValue(Pop());
 
   } else if (prop != NULL) {
     if (prop->key()->IsPropertyName()) {
       // Named property.
       CHECK_ALIVE(VisitForValue(prop->obj()));
       HValue* object = Top();
 
       Handle<String> name = prop->key()->AsLiteral()->AsPropertyName();
       Handle<Map> map;
-      HInstruction* load;
+      HInstruction* load = NULL;
+      SmallMapList* types = prop->GetReceiverTypes();
       bool monomorphic = prop->IsMonomorphic();
       if (monomorphic) {
-        map = prop->GetReceiverTypes()->first();
+        map = types->first();
         // We can't generate code for a monomorphic dict mode load so
         // just pretend it is not monomorphic.
         if (map->is_dictionary_map()) monomorphic = false;
       }
       if (monomorphic) {

[Hannes Payer (out of office), 2013/06/13 15:26:36]

can you restructure the code 
  monomorphic = false;
} 
if (monomorphic) {
...

[Toon Verwaest, 2013/06/13 16:33:58]

It kinda minimizes the number of branches. We should probably refactor it... but
not now :)

On 2013/06/13 15:26:36, Hannes Payer wrote:

         Handle<JSFunction> getter;
         Handle<JSObject> holder;
         if (LookupGetter(map, name, &getter, &holder)) {
           load = BuildCallGetter(object, map, getter, holder);
         } else {
           load = BuildLoadNamedMonomorphic(object, name, prop, map);
         }
-      } else {
-        load = BuildLoadNamedGeneric(object, name, prop);
+      } else if (types != NULL && types->length() > 1) {
+        load = LoadPolymorphicAsMonomorphic(prop, object, types, name);
       }
+      if (load == NULL) load = BuildLoadNamedGeneric(object, name, prop);
       PushAndAdd(load);
       if (load->HasObservableSideEffects()) {
         AddSimulate(prop->LoadId(), REMOVABLE_SIMULATE);
       }
 
       CHECK_ALIVE(VisitForValue(expr->value()));
       HValue* right = Pop();
       HValue* left = Pop();
 
       HInstruction* instr = BuildBinaryOperation(operation, left, right);
@@ skipping 245 matching lines @@
   return field;
 }
 
 
 HInstruction* HOptimizedGraphBuilder::BuildLoadNamedGeneric(
     HValue* object,
     Handle<String> name,
     Property* expr) {
   if (expr->IsUninitialized()) {
     AddSoftDeoptimize();
+  } else {
+    // OS::DebugBreak();

[Hannes Payer (out of office), 2013/06/13 15:26:36]

I guess you do not need that anymore.

[Toon Verwaest, 2013/06/13 16:33:58]

Done.

   }
   HValue* context = environment()->LookupContext();
   return new(zone()) HLoadNamedGeneric(context, object, name);
 }
 
 
 HInstruction* HOptimizedGraphBuilder::BuildCallGetter(
     HValue* object,
     Handle<Map> map,
     Handle<JSFunction> getter,
@@ skipping 2278 matching lines @@
 
     if (prop->key()->IsPropertyName()) {
       // Named property.
       if (returns_original_input) Push(graph()->GetConstantUndefined());
 
       CHECK_ALIVE(VisitForValue(prop->obj()));
       HValue* object = Top();
 
       Handle<String> name = prop->key()->AsLiteral()->AsPropertyName();
       Handle<Map> map;
-      HInstruction* load;
+      HInstruction* load = NULL;
       bool monomorphic = prop->IsMonomorphic();
+      SmallMapList* types = prop->GetReceiverTypes();
       if (monomorphic) {
-        map = prop->GetReceiverTypes()->first();
+        map = types->first();
         if (map->is_dictionary_map()) monomorphic = false;
       }
       if (monomorphic) {

[Hannes Payer (out of office), 2013/06/13 15:26:36]

can you restructure the code 
  monomorphic = false;
} 
if (monomorphic) {
...

[Toon Verwaest, 2013/06/13 16:33:58]

Same thing here

On 2013/06/13 15:26:36, Hannes Payer wrote:

         Handle<JSFunction> getter;
         Handle<JSObject> holder;
         if (LookupGetter(map, name, &getter, &holder)) {
           load = BuildCallGetter(object, map, getter, holder);
         } else {
           load = BuildLoadNamedMonomorphic(object, name, prop, map);
         }
-      } else {
-        load = BuildLoadNamedGeneric(object, name, prop);
+      } else if (types != NULL && types->length() > 1) {
+        load = LoadPolymorphicAsMonomorphic(prop, object, types, name);
       }
+      if (load == NULL) load = BuildLoadNamedGeneric(object, name, prop);

[Hannes Payer (out of office), 2013/06/13 15:26:36]

This sequence of code is exactly the same as in line 6666. I think it makes
sense to move it into a separate function. WDYT?

[Toon Verwaest, 2013/06/13 16:33:58]

True, but lets do that in a different CL.

On 2013/06/13 15:26:36, Hannes Payer wrote:

       PushAndAdd(load);
       if (load->HasObservableSideEffects()) {
         AddSimulate(prop->LoadId(), REMOVABLE_SIMULATE);
       }
 
       after = BuildIncrement(returns_original_input, expr);
       input = Pop();
 
       HInstruction* store;
       if (!monomorphic || map->is_observed()) {
@@ skipping 2319 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify(false);  // No full verify.
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal