[turbofan] Further optimize spread/apply with arguments/rest parameters.

src/compiler/js-call-reducer.cc

Index: src/compiler/js-call-reducer.cc
diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc
index 79fd6f6d3862d405ef90330c4526bcb957027fb4..dd3476835693682f1f04a11f72dbbaabeabe5db8 100644
--- a/src/compiler/js-call-reducer.cc
+++ b/src/compiler/js-call-reducer.cc
@@ -40,6 +40,23 @@ Reduction JSCallReducer::Reduce(Node* node) {
   return NoChange();
 }
 
+void JSCallReducer::Finalize() {

[petermarshall, 2017/06/29 13:25:24]

I'm trying to figure out how we guarantee there is no infinite loop here:

- It seems like finalize can be called more than once.
- Could the reduction below trigger more things to be added to the waitlist?

How do we keep track of all that?

[Benedikt Meurer, 2017/06/30 04:38:08]

Finalize can be called multiple times, but only if there's a regular reduction
in between. So unless any of the items in the waitlist trigger a reduction (i.e.
we make progress on at least one node), the Finalize logic will not be invoked
again.

+  // TODO(turbofan): This is not the best solution; ideally we would be able
+  // to teach the GraphReducer about arbitrary dependencies between different
+  // nodes, even if they don't show up in the use list of the other node.
+  std::set<Node*> const waitlist = std::move(waitlist_);

[petermarshall, 2017/06/29 13:25:24]

I don't think std::move does anything useful here, but you'd have to defer to
someone with stronger c++ foo than I (clemensh perhaps).

[Benedikt Meurer, 2017/06/30 04:38:08]

It is the important bit, as it clears the waitlist_ and transfers all the items
to waitlist.

+  for (Node* node : waitlist) {
+    if (!node->IsDead()) {
+      Reduction const reduction = Reduce(node);
+      if (reduction.Changed()) {
+        Node* replacement = reduction.replacement();
+        if (replacement != node) {
+          Replace(node, replacement);
+        }
+      }
+    }
+  }
+}
 
 // ES6 section 22.1.1 The Array Constructor
 Reduction JSCallReducer::ReduceArrayConstructor(Node* node) {
@@ -686,6 +703,23 @@ Reduction JSCallReducer::ReduceCallApiFunction(
   return Changed(node);
 }
 
+namespace {
+
+// Check whether elements aren't mutated; we play it extremely safe here by
+// explicitly checking that {node} only used by {LoadField} or {LoadElement}.

[petermarshall, 2017/06/29 13:25:24]

Nit: ...{node} *is* only...

[Benedikt Meurer, 2017/06/30 07:21:36]

Done.

+bool IsSafeArgumentsElements(Node* node) {
+  for (Edge const edge : node->use_edges()) {
+    if (!NodeProperties::IsValueEdge(edge)) continue;
+    if (edge.from()->opcode() != IrOpcode::kLoadField &&
+        edge.from()->opcode() != IrOpcode::kLoadElement) {
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace
+
 Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
     Node* node, int arity, CallFrequency const& frequency) {
   DCHECK(node->opcode() == IrOpcode::kJSCallWithArrayLike ||
@@ -704,19 +738,63 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
   // Check if {arguments_list} is an arguments object, and {node} is the only
   // value user of {arguments_list} (except for value uses in frame states).
   Node* arguments_list = NodeProperties::GetValueInput(node, arity);
-  if (arguments_list->opcode() != IrOpcode::kJSCreateArguments)
+  if (arguments_list->opcode() != IrOpcode::kJSCreateArguments) {
     return NoChange();
+  }
   for (Edge edge : arguments_list->use_edges()) {
+    if (!NodeProperties::IsValueEdge(edge)) continue;
     Node* const user = edge.from();
-    if (user == node) continue;
-    // Ignore uses as frame state's locals or parameters.
-    if (user->opcode() == IrOpcode::kStateValues) continue;
-    // Ignore uses as frame state's accumulator.
-    if (user->opcode() == IrOpcode::kFrameState &&
-        user->InputAt(2) == arguments_list) {
-      continue;
+    switch (user->opcode()) {
+      case IrOpcode::kCheckMaps:
+      case IrOpcode::kFrameState:
+      case IrOpcode::kStateValues:
+      case IrOpcode::kReferenceEqual:
+      case IrOpcode::kReturn:
+        // Ignore safe uses that definitely don't mess with the arguments.
+        continue;
+      case IrOpcode::kLoadField: {
+        DCHECK_EQ(arguments_list, user->InputAt(0));
+        FieldAccess const& access = FieldAccessOf(user->op());
+        if (access.offset == JSArray::kLengthOffset) {

[petermarshall, 2017/06/29 13:25:24]

If this is checking for LoadField on an arguments object, and we want to detect
and whitelist accesses to the length, then should we just check access.offset ==
JSArgumentsObject::kLengthOffset ?
Why the equivalence with JSArray::kLengthOffset?

[Benedikt Meurer, 2017/06/30 04:39:37]

Rest parameters are JSArray instances.

[petermarshall, 2017/06/30 07:14:10]

OK

+          // Ignore uses for arguments#length.
+          STATIC_ASSERT(JSArray::kLengthOffset ==
+                        JSArgumentsObject::kLengthOffset);
+          continue;
+        } else if (access.offset == JSObject::kElementsOffset) {
+          // Ignore safe uses for arguments#elements.
+          if (IsSafeArgumentsElements(user)) continue;
+        }
+        break;
+      }
+      case IrOpcode::kJSCallWithArrayLike:
+        // Ignore uses as argumentsList input to calls with array like.
+        if (user->InputAt(2) == arguments_list) continue;
+        break;
+      case IrOpcode::kJSConstructWithArrayLike:
+        // Ignore uses as argumentsList input to calls with array like.
+        if (user->InputAt(1) == arguments_list) continue;
+        break;
+      case IrOpcode::kJSCallWithSpread: {
+        // Ignore uses as spread input to calls with spread.
+        SpreadWithArityParameter p = SpreadWithArityParameterOf(user->op());
+        int const arity = static_cast<int>(p.arity() - 1);
+        if (user->InputAt(arity) == arguments_list) continue;
+        break;
+      }
+      case IrOpcode::kJSConstructWithSpread: {
+        // Ignore uses as spread input to construct with spread.
+        SpreadWithArityParameter p = SpreadWithArityParameterOf(user->op());
+        int const arity = static_cast<int>(p.arity() - 2);
+        if (user->InputAt(arity) == arguments_list) continue;
+        break;
+      }
+      default:
+        break;
     }
-    if (!NodeProperties::IsValueEdge(edge)) continue;
+    // We cannot currently reduce the {node} to something better than what
+    // it already is, but we might be able to do something about the {node}
+    // later, so put it on the waitlist and try again during finalization.
+    waitlist_.insert(node);
     return NoChange();
   }