[turbofan] Don't introduce unnecessary map checks.

Introduce NodeProperties::NoObservableSideEffectBetween to check if
there's any observable side effect between two nodes in the effect
chain. Use this to guard the insertion of potentially redundant map
checks in the lowering of Object.prototype.hasOwnProperty and keyed
accesses within a for..in loop. This gives another boost on the for..in
performance front.

Bug: v8:6702
Change-Id: I68133f14ad388a1a7422714319c9b323d5cf8bc4
Reviewed-on: https://chromium-review.googlesource.com/654640Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47869}

src/compiler/js-call-reducer.cc

@@ -464,7 +464,10 @@ Reduction JSCallReducer::ReduceObjectPrototypeHasOwnProperty(Node* node) {
 
   // We can constant-fold the {node} to True in this case, and insert
   // a (potentially redundant) map check to guard the fact that the
-  // {receiver} map didn't change since the dominating JSForInNext.
+  // {receiver} map didn't change since the dominating JSForInNext. This
+  // map check is only necessary when TurboFan cannot prove that there
+  // is no observable side effect between the {JSForInNext} and the
+  // {JSCall} to Object.prototype.hasOwnProperty.
   //
   // Also note that it's safe to look through the {JSToObject}, since the
   // Object.prototype.hasOwnProperty does an implicit ToObject anyway, and
@@ -478,13 +481,17 @@ Reduction JSCallReducer::ReduceObjectPrototypeHasOwnProperty(Node* node) {
         object = NodeProperties::GetValueInput(object, 0);
       }
       if (object == receiver) {
-        Node* receiver_map = effect =
-            graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
-                             receiver, effect, control);
-        Node* check = graph()->NewNode(simplified()->ReferenceEqual(),
-                                       receiver_map, cache_type);
-        effect =
-            graph()->NewNode(simplified()->CheckIf(), check, effect, control);
+        // No need to repeat the map check if we can prove that there's no
+        // observable side effect between {effect} and {name].
+        if (!NodeProperties::NoObservableSideEffectBetween(effect, name)) {
+          Node* receiver_map = effect =
+              graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
+                               receiver, effect, control);
+          Node* check = graph()->NewNode(simplified()->ReferenceEqual(),
+                                         receiver_map, cache_type);
+          effect =
+              graph()->NewNode(simplified()->CheckIf(), check, effect, control);
+        }
         Node* value = jsgraph()->TrueConstant();
         ReplaceWithValue(node, value, effect, control);
         return Replace(value);
@@ -1212,12 +1219,9 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
     // TODO(turbofan): Further relax this constraint.
     if (formal_parameter_count != 0) {
       Node* effect = NodeProperties::GetEffectInput(node);
-      while (effect != arguments_list) {
-        if (effect->op()->EffectInputCount() != 1 ||
-            !(effect->op()->properties() & Operator::kNoWrite)) {
-          return NoChange();
-        }
-        effect = NodeProperties::GetEffectInput(effect);
+      if (!NodeProperties::NoObservableSideEffectBetween(effect,
+                                                         arguments_list)) {
+        return NoChange();
       }
     }
   } else if (type == CreateArgumentsType::kRestParameter) {

src/compiler/js-native-context-specialization.cc

@@ -1412,7 +1412,9 @@ Reduction JSNativeContextSpecialization::ReduceJSLoadProperty(Node* node) {
   // If the for..in has only seen maps with enum cache consisting of keys
   // and indices so far, we can turn the {JSLoadProperty} into a map check
   // on the {receiver} and then just load the field value dynamically via
-  // the {LoadFieldByIndex} operator.
+  // the {LoadFieldByIndex} operator. The map check is only necessary when
+  // TurboFan cannot prove that there is no observable side effect between
+  // the {JSForInNext} and the {JSLoadProperty} node.
   //
   // Also note that it's safe to look through the {JSToObject}, since the
   // [[Get]] operation does an implicit ToObject anyway, and these operations
@@ -1427,8 +1429,48 @@ Reduction JSNativeContextSpecialization::ReduceJSLoadProperty(Node* node) {
         object = NodeProperties::GetValueInput(object, 0);
       }
       if (object == receiver) {
-        Node* value = effect =
-            BuildForInNextValue(receiver, enumerator, index, effect, control);
+        // No need to repeat the map check if we can prove that there's no
+        // observable side effect between {effect} and {name].
+        if (!NodeProperties::NoObservableSideEffectBetween(effect, name)) {
+          // Check that the {receiver} map is still valid.
+          Node* receiver_map = effect =
+              graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
+                               receiver, effect, control);
+          Node* check = graph()->NewNode(simplified()->ReferenceEqual(),
+                                         receiver_map, enumerator);
+          effect =
+              graph()->NewNode(simplified()->CheckIf(), check, effect, control);
+        }
+
+        // Load the enum cache indices from the {cache_type}.
+        Node* descriptor_array = effect = graph()->NewNode(
+            simplified()->LoadField(AccessBuilder::ForMapDescriptors()),
+            enumerator, effect, control);
+        Node* enum_cache = effect =
+            graph()->NewNode(simplified()->LoadField(
+                                 AccessBuilder::ForDescriptorArrayEnumCache()),
+                             descriptor_array, effect, control);
+        Node* enum_indices = effect = graph()->NewNode(
+            simplified()->LoadField(AccessBuilder::ForEnumCacheIndices()),
+            enum_cache, effect, control);
+
+        // Ensure that the {enum_indices} are valid.
+        Node* check = graph()->NewNode(
+            simplified()->BooleanNot(),
+            graph()->NewNode(simplified()->ReferenceEqual(), enum_indices,
+                             jsgraph()->EmptyFixedArrayConstant()));
+        effect =
+            graph()->NewNode(simplified()->CheckIf(), check, effect, control);
+
+        // Determine the index from the {enum_indices}.
+        index = effect = graph()->NewNode(
+            simplified()->LoadElement(
+                AccessBuilder::ForFixedArrayElement(PACKED_SMI_ELEMENTS)),
+            enum_indices, index, effect, control);
+
+        // Load the actual field value.
+        Node* value = effect = graph()->NewNode(
+            simplified()->LoadFieldByIndex(), receiver, index, effect, control);
         ReplaceWithValue(node, value, effect, control);
         return Replace(value);
       }
@@ -2337,48 +2379,6 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
   return graph()->NewNode(common()->FinishRegion(), new_properties, effect);
 }
 
-Node* JSNativeContextSpecialization::BuildForInNextValue(Node* receiver,
-                                                         Node* enumerator,
-                                                         Node* index,
-                                                         Node* effect,
-                                                         Node* control) {
-  // Check that the {receiver} map is still valid.
-  Node* receiver_map = effect =
-      graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
-                       receiver, effect, control);
-  Node* check = graph()->NewNode(simplified()->ReferenceEqual(), receiver_map,
-                                 enumerator);
-  effect = graph()->NewNode(simplified()->CheckIf(), check, effect, control);
-
-  // Load the enum cache indices from the {cache_type}.
-  Node* descriptor_array = effect = graph()->NewNode(
-      simplified()->LoadField(AccessBuilder::ForMapDescriptors()), enumerator,
-      effect, control);
-  Node* enum_cache = effect = graph()->NewNode(
-      simplified()->LoadField(AccessBuilder::ForDescriptorArrayEnumCache()),
-      descriptor_array, effect, control);
-  Node* enum_indices = effect = graph()->NewNode(
-      simplified()->LoadField(AccessBuilder::ForEnumCacheIndices()), enum_cache,
-      effect, control);
-
-  // Ensure that the {enum_indices} are valid.
-  check = graph()->NewNode(
-      simplified()->BooleanNot(),
-      graph()->NewNode(simplified()->ReferenceEqual(), enum_indices,
-                       jsgraph()->EmptyFixedArrayConstant()));
-  effect = graph()->NewNode(simplified()->CheckIf(), check, effect, control);
-
-  // Determine the index from the {enum_indices}.
-  index = effect = graph()->NewNode(
-      simplified()->LoadElement(
-          AccessBuilder::ForFixedArrayElement(PACKED_SMI_ELEMENTS)),
-      enum_indices, index, effect, control);
-
-  // Load the actual field value.
-  return graph()->NewNode(simplified()->LoadFieldByIndex(), receiver, index,
-                          effect, control);
-}
-
 bool JSNativeContextSpecialization::CanTreatHoleAsUndefined(
     MapHandles const& receiver_maps) {
   // Check if all {receiver_maps} either have one of the initial Array.prototype

src/compiler/js-native-context-specialization.h

@@ -162,11 +162,6 @@ class JSNativeContextSpecialization final : public AdvancedReducer {
   Node* BuildExtendPropertiesBackingStore(Handle<Map> map, Node* properties,
                                           Node* effect, Node* control);
 
-  // Construct appropriate subgraph to read the next value in a fast for-in
-  // iteration.
-  Node* BuildForInNextValue(Node* receiver, Node* enumerator, Node* index,
-                            Node* effect, Node* control);
-
   // Checks if we can turn the hole into undefined when loading an element
   // from an object with one of the {receiver_maps}; sets up appropriate
   // code dependencies and might use the array protector cell.

src/compiler/node-properties.cc

@@ -456,6 +456,20 @@ NodeProperties::InferReceiverMapsResult NodeProperties::InferReceiverMaps(
   }
 }
 
+// static
+bool NodeProperties::NoObservableSideEffectBetween(Node* effect,
+                                                   Node* dominator) {
+  while (effect != dominator) {
+    if (effect->op()->EffectInputCount() == 1 &&
+        effect->op()->properties() & Operator::kNoWrite) {
+      effect = NodeProperties::GetEffectInput(effect);
+    } else {
+      return false;
+    }
+  }
+  return true;
+}
+
 // static
 Node* NodeProperties::GetOuterContext(Node* node, size_t* depth) {
   Node* context = NodeProperties::GetContextInput(node);

src/compiler/node-properties.h

@@ -150,6 +150,11 @@ class V8_EXPORT_PRIVATE NodeProperties final {
   static InferReceiverMapsResult InferReceiverMaps(
       Node* receiver, Node* effect, ZoneHandleSet<Map>* maps_return);
 
+  // Walks up the {effect} chain to check that there's no observable side-effect
+  // between the {effect} and it's {dominator}. Aborts the walk if there's join
+  // in the effect chain.
+  static bool NoObservableSideEffectBetween(Node* effect, Node* dominator);
+
   // ---------------------------------------------------------------------------
   // Context.