Pass checked values to HLoadNamedField, removing the need for extra type-check field.

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 4cd0ff0f08658ad651ac99c4552b9d1100a380a3..188c4a642bc24ec48d8e28a11f10506fbe121521 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -1191,7 +1191,7 @@ void HGraphBuilder::BuildTransitionElementsKind(HValue* object,
   }
 
   if (!IsSimpleMapChangeTransition(from_kind, to_kind)) {
-    HInstruction* elements = AddLoadElements(object, NULL);
+    HInstruction* elements = AddLoadElements(object);
 
     HInstruction* empty_fixed_array = Add<HConstant>(
         isolate()->factory()->empty_fixed_array());
@@ -1222,7 +1222,7 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
     HValue* object,
     HValue* key,
     HValue* val,
-    HCheckMaps* mapcheck,
+    HCheckMaps* checked_value,

[titzer, 2013/08/21 14:07:56]

s/checked_value/checked_object/g ?

[Toon Verwaest, 2013/08/21 14:28:43]

Done.

     bool is_js_array,
     ElementsKind elements_kind,
     bool is_store,
@@ -1237,13 +1237,14 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
   // generated store code.
   if ((elements_kind == FAST_HOLEY_ELEMENTS) ||
       (elements_kind == FAST_ELEMENTS && is_store)) {
-    if (mapcheck != NULL) {
-      mapcheck->ClearGVNFlag(kDependsOnElementsKind);
+    if (checked_value != NULL) {
+      checked_value->ClearGVNFlag(kDependsOnElementsKind);
     }
   }
+  if (checked_value != NULL) object = checked_value;
   bool fast_smi_only_elements = IsFastSmiElementsKind(elements_kind);
   bool fast_elements = IsFastObjectElementsKind(elements_kind);
-  HValue* elements = AddLoadElements(object, mapcheck);
+  HValue* elements = AddLoadElements(object);
   if (is_store && (fast_elements || fast_smi_only_elements) &&
       store_mode != STORE_NO_TRANSITION_HANDLE_COW) {
     HCheckMaps* check_cow_map = Add<HCheckMaps>(
@@ -1252,8 +1253,8 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
   }
   HInstruction* length = NULL;
   if (is_js_array) {
-    length = Add<HLoadNamedField>(object,
-        HObjectAccess::ForArrayLength(elements_kind), mapcheck);
+    length = Add<HLoadNamedField>(
+        object, HObjectAccess::ForArrayLength(elements_kind));
   } else {
     length = AddLoadFixedArrayLength(elements);
   }
@@ -1283,7 +1284,7 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
           Add<HLoadExternalArrayPointer>(elements);
       return AddExternalArrayElementAccess(
           external_elements, checked_key, val,
-          mapcheck, elements_kind, is_store);
+          checked_value, elements_kind, is_store);
     }
   }
   ASSERT(fast_smi_only_elements ||
@@ -1320,7 +1321,7 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess(
       }
     }
   }
-  return AddFastElementAccess(elements, checked_key, val, mapcheck,
+  return AddFastElementAccess(elements, checked_key, val, checked_value,
                               elements_kind, is_store, load_mode, store_mode);
 }
 
@@ -1493,11 +1494,8 @@ HInstruction* HGraphBuilder::AddFastElementAccess(
 }
 
 
-HLoadNamedField* HGraphBuilder::AddLoadElements(HValue* object,
-                                                HValue* typecheck) {
-  return Add<HLoadNamedField>(object,
-                              HObjectAccess::ForElementsPointer(),
-                              typecheck);
+HLoadNamedField* HGraphBuilder::AddLoadElements(HValue* object) {
+  return Add<HLoadNamedField>(object, HObjectAccess::ForElementsPointer());
 }
 
 
@@ -1700,7 +1698,7 @@ HValue* HGraphBuilder::BuildCloneShallowArray(HValue* boilerplate,
   if (length > 0) {
     // Get hold of the elements array of the boilerplate and setup the
     // elements pointer in the resulting object.
-    HValue* boilerplate_elements = AddLoadElements(boilerplate, NULL);
+    HValue* boilerplate_elements = AddLoadElements(boilerplate);
     HValue* object_elements = Add<HInnerAllocatedObject>(object, elems_offset);
     Add<HStoreNamedField>(object, HObjectAccess::ForElementsPointer(),
                           object_elements);
@@ -1833,7 +1831,7 @@ HValue* HGraphBuilder::JSArrayBuilder::EmitMapCode() {
     // map, because we can just load the map in that case.
     HObjectAccess access = HObjectAccess::ForPrototypeOrInitialMap();
     return builder()->AddInstruction(
-        builder()->BuildLoadNamedField(constructor_function_, access, NULL));
+        builder()->BuildLoadNamedField(constructor_function_, access));
   }
 
   HInstruction* native_context = builder()->BuildGetNativeContext();
@@ -1854,7 +1852,7 @@ HValue* HGraphBuilder::JSArrayBuilder::EmitInternalMapCode() {
   // Find the map near the constructor function
   HObjectAccess access = HObjectAccess::ForPrototypeOrInitialMap();
   return builder()->AddInstruction(
-      builder()->BuildLoadNamedField(constructor_function_, access, NULL));
+      builder()->BuildLoadNamedField(constructor_function_, access));
 }
 
 
@@ -4298,7 +4296,6 @@ void HOptimizedGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) {
   int data_size = 0;
   int pointer_size = 0;
   int max_properties = kMaxFastLiteralProperties;
-  HCheckMaps* type_check = NULL;
   if (IsFastLiteral(original_boilerplate_object,
                     kMaxFastLiteralDepth,
                     &max_properties,
@@ -4336,7 +4333,7 @@ void HOptimizedGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) {
     // De-opt if elements kind changed from boilerplate_elements_kind.
     Handle<Map> map = Handle<Map>(original_boilerplate_object->map(),
                                   isolate());
-    type_check = Add<HCheckMaps>(literal, map, top_info());
+    literal = Add<HCheckMaps>(literal, map, top_info());
   }
 
   // The array is expected in the bailout environment during computation
@@ -4357,7 +4354,7 @@ void HOptimizedGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) {
     HValue* value = Pop();
     if (!Smi::IsValid(i)) return Bailout(kNonSmiKeyInArrayLiteral);
 
-    elements = AddLoadElements(literal, type_check);
+    elements = AddLoadElements(literal);
 
     HValue* key = Add<HConstant>(i);
 
@@ -4580,8 +4577,8 @@ HInstruction* HOptimizedGraphBuilder::TryLoadPolymorphicAsMonomorphic(
   if (count == types->length()) {
     // Everything matched; can use monomorphic load.
     BuildCheckHeapObject(object);
-    HCheckMaps* type_check = Add<HCheckMaps>(object, types);
-    return BuildLoadNamedField(object, access, type_check);
+    HCheckMaps* checked_value = Add<HCheckMaps>(object, types);
+    return BuildLoadNamedField(checked_value, access);
   }
 
   if (count != 0) return NULL;
@@ -4602,14 +4599,14 @@ HInstruction* HOptimizedGraphBuilder::TryLoadPolymorphicAsMonomorphic(
   if (!lookup.IsField()) return NULL;
 
   BuildCheckHeapObject(object);
-  HCheckMaps* type_check = Add<HCheckMaps>(object, types);
+  Add<HCheckMaps>(object, types);

[titzer, 2013/08/21 14:07:56]

I'm not quite sure where the result of this guy should be fed. Who consumes the
object afterwards?

[Toon Verwaest, 2013/08/21 14:28:43]

The problem is that we have a checkmap, and then we could load the prototype
from the map; but given that it's a constant, we rather just inline the
prototype as a constant directly. An HConstant doesn't have a dependency;
because it's constant anyway.

I agree, the problem is that the prototype is only that constant if the
HCheckMaps of the receiver succeeded. This goes even further however. When we
check the prototype chain, we have a list of HCheckMaps(HConstant(p),
expected_map_for_p);, which can even be removed (replaced by a code dependency)
if the map of p is "stable".

This CL doesn't fix that problem; but it does at least let the HLoadNamedField
below depend on the right mapcheck; the check of the prototype from which we
load the value.

On 2013/08/21 14:07:56, titzer wrote:

 
   Handle<JSObject> holder(lookup.holder());
   Handle<Map> holder_map(holder->map());
-  BuildCheckPrototypeMaps(Handle<JSObject>::cast(prototype), holder);
-  HValue* holder_value = Add<HConstant>(holder);
-  return BuildLoadNamedField(holder_value,
-      HObjectAccess::ForField(holder_map, &lookup, name), type_check);
+  HValue* checked_holder = BuildCheckPrototypeMaps(
+      Handle<JSObject>::cast(prototype), holder);
+  return BuildLoadNamedField(checked_holder,
+      HObjectAccess::ForField(holder_map, &lookup, name));
 }
 
 
@@ -4684,7 +4681,7 @@ void HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(
       // TODO(verwaest): Merge logic with BuildLoadNamedMonomorphic.
       if (lookup.IsField()) {
         HObjectAccess access = HObjectAccess::ForField(map, &lookup, name);
-        HLoadNamedField* load = BuildLoadNamedField(object, access, compare);
+        HLoadNamedField* load = BuildLoadNamedField(compare, access);
         load->set_position(expr->position());
         AddInstruction(load);
         if (!ast_context()->IsEffect()) Push(load);
@@ -5357,32 +5354,29 @@ void HOptimizedGraphBuilder::VisitThrow(Throw* expr) {
 
 
 HLoadNamedField* HGraphBuilder::BuildLoadNamedField(HValue* object,
-                                                    HObjectAccess access,
-                                                    HValue* typecheck) {
+                                                    HObjectAccess access) {
   if (FLAG_track_double_fields && access.representation().IsDouble()) {
     // load the heap number
     HLoadNamedField* heap_number = Add<HLoadNamedField>(
         object, access.WithRepresentation(Representation::Tagged()));
     heap_number->set_type(HType::HeapNumber());
     // load the double value from it
-    return New<HLoadNamedField>(heap_number,
-                                HObjectAccess::ForHeapNumberValue(),
-                                typecheck);
+    return New<HLoadNamedField>(
+        heap_number, HObjectAccess::ForHeapNumberValue());
   }
-  return New<HLoadNamedField>(object, access, typecheck);
+  return New<HLoadNamedField>(object, access);
 }
 
 
 HInstruction* HGraphBuilder::BuildLoadStringLength(HValue* object,
-                                                   HValue* typecheck) {
+                                                   HValue* checked_string) {
   if (FLAG_fold_constants && object->IsConstant()) {
     HConstant* constant = HConstant::cast(object);
     if (constant->HasStringValue()) {
       return New<HConstant>(constant->StringValue()->length());
     }
   }
-  return BuildLoadNamedField(
-      object, HObjectAccess::ForStringLength(), typecheck);
+  return BuildLoadNamedField(checked_string, HObjectAccess::ForStringLength());
 }
 
 
@@ -5421,18 +5415,18 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadNamedMonomorphic(
   // Handle access to various length properties
   if (name->Equals(isolate()->heap()->length_string())) {
     if (map->instance_type() == JS_ARRAY_TYPE) {
-      HCheckMaps* type_check = AddCheckMap(object, map);
-      return New<HLoadNamedField>(object,
-          HObjectAccess::ForArrayLength(map->elements_kind()), type_check);
+      HCheckMaps* checked_value = AddCheckMap(object, map);
+      return New<HLoadNamedField>(
+          checked_value, HObjectAccess::ForArrayLength(map->elements_kind()));
     }
   }
 
   LookupResult lookup(isolate());
   map->LookupDescriptor(NULL, *name, &lookup);
   if (lookup.IsField()) {
-    HCheckMaps* type_check = AddCheckMap(object, map);
-    return BuildLoadNamedField(object,
-        HObjectAccess::ForField(map, &lookup, name), type_check);
+    HCheckMaps* checked_value = AddCheckMap(object, map);
+    return BuildLoadNamedField(
+        checked_value, HObjectAccess::ForField(map, &lookup, name));
   }
 
   // Handle a load of a constant known function.
@@ -5448,11 +5442,10 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadNamedMonomorphic(
     Handle<JSObject> prototype(JSObject::cast(map->prototype()));
     Handle<JSObject> holder(lookup.holder());
     Handle<Map> holder_map(holder->map());
-    HCheckMaps* type_check = AddCheckMap(object, map);
-    BuildCheckPrototypeMaps(prototype, holder);
-    HValue* holder_value = Add<HConstant>(holder);
-    return BuildLoadNamedField(holder_value,
-        HObjectAccess::ForField(holder_map, &lookup, name), type_check);
+    AddCheckMap(object, map);
+    HValue* checked_holder = BuildCheckPrototypeMaps(prototype, holder);
+    return BuildLoadNamedField(
+        checked_holder, HObjectAccess::ForField(holder_map, &lookup, name));
   }
 
   // Handle a load of a constant function somewhere in the prototype chain.
@@ -5645,11 +5638,11 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess(
     return is_store ? NULL : instr;
   }
 
-  HInstruction* checkspec =
+  HInstruction* checked_value =
       AddInstruction(HCheckInstanceType::NewIsSpecObject(object, zone()));
   HBasicBlock* join = graph()->CreateBasicBlock();
 
-  HInstruction* elements = AddLoadElements(object, checkspec);
+  HInstruction* elements = AddLoadElements(checked_value);
 
   for (int i = 0; i < untransitionable_maps.length(); ++i) {
     Handle<Map> map = untransitionable_maps[i];
@@ -5671,7 +5664,7 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess(
       }
       if (map->instance_type() == JS_ARRAY_TYPE) {
         HInstruction* length = Add<HLoadNamedField>(
-            object, HObjectAccess::ForArrayLength(elements_kind), mapcompare);
+            mapcompare, HObjectAccess::ForArrayLength(elements_kind));
         checked_key = Add<HBoundsCheck>(key, length);
       } else {
         HInstruction* length = AddLoadFixedArrayLength(elements);
@@ -5947,30 +5940,34 @@ void HOptimizedGraphBuilder::VisitProperty(Property* expr) {
 }
 
 
-void HGraphBuilder::BuildConstantMapCheck(Handle<JSObject> constant,
-                                          CompilationInfo* info) {
+HInstruction* HGraphBuilder::BuildConstantMapCheck(Handle<JSObject> constant,
+                                                   CompilationInfo* info) {
   HConstant* constant_value = New<HConstant>(constant);
 
   if (constant->map()->CanOmitMapChecks()) {
     constant->map()->AddDependentCompilationInfo(
         DependentCode::kPrototypeCheckGroup, info);
-    return;
+    return constant_value;
   }
 
   AddInstruction(constant_value);
   HCheckMaps* check =
       Add<HCheckMaps>(constant_value, handle(constant->map()), info);
   check->ClearGVNFlag(kDependsOnElementsKind);
+  return check;
 }
 
 
-void HGraphBuilder::BuildCheckPrototypeMaps(Handle<JSObject> prototype,
-                                            Handle<JSObject> holder) {
-  BuildConstantMapCheck(prototype, top_info());
+HInstruction* HGraphBuilder::BuildCheckPrototypeMaps(Handle<JSObject> prototype,
+                                                     Handle<JSObject> holder) {
   while (!prototype.is_identical_to(holder)) {
-    prototype = handle(JSObject::cast(prototype->GetPrototype()));
     BuildConstantMapCheck(prototype, top_info());
+    prototype = handle(JSObject::cast(prototype->GetPrototype()));
   }
+
+  HInstruction* checked_value = BuildConstantMapCheck(prototype, top_info());
+  if (!checked_value->IsLinked()) AddInstruction(checked_value);
+  return checked_value;
 }