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

src/compiler/js-call-reducer.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/compiler/js-call-reducer.h"
 
 #include "src/code-factory.h"
 #include "src/code-stubs.h"
 #include "src/compilation-dependencies.h"
 #include "src/compiler/access-builder.h"
@@ skipping 22 matching lines @@
     case IrOpcode::kJSCallWithArrayLike:
       return ReduceJSCallWithArrayLike(node);
     case IrOpcode::kJSCallWithSpread:
       return ReduceJSCallWithSpread(node);
     default:
       break;
   }
   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) {
   DCHECK_EQ(IrOpcode::kJSCall, node->opcode());
   Node* target = NodeProperties::GetValueInput(node, 0);
   CallParameters const& p = CallParametersOf(node->op());
 
   // Check if we have an allocation site from the CallIC.
   Handle<AllocationSite> site;
   if (p.feedback().IsValid()) {
@@ skipping 626 matching lines @@
                     jsgraph()->HeapConstant(stub.GetCode()));
   node->InsertInput(graph()->zone(), 2, jsgraph()->Constant(data));
   node->InsertInput(graph()->zone(), 3, holder);
   node->InsertInput(graph()->zone(), 4,
                     jsgraph()->ExternalConstant(function_reference));
   node->ReplaceInput(5, receiver);
   NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
   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 ||
          node->opcode() == IrOpcode::kJSCallWithSpread ||
          node->opcode() == IrOpcode::kJSConstructWithArrayLike ||
          node->opcode() == IrOpcode::kJSConstructWithSpread);
 
   // In case of a call/construct with spread, we need to
   // ensure that it's safe to avoid the actual iteration.
   if ((node->opcode() == IrOpcode::kJSCallWithSpread ||
        node->opcode() == IrOpcode::kJSConstructWithSpread) &&
       !isolate()->initial_array_iterator_prototype_map()->is_stable()) {
     return NoChange();
   }
 
   // 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();
   }
 
   // Get to the actual frame state from which to extract the arguments;
   // we can only optimize this in case the {node} was already inlined into
   // some other function (and same for the {arguments_list}).
   CreateArgumentsType const type = CreateArgumentsTypeOf(arguments_list->op());
   Node* frame_state = NodeProperties::GetFrameStateInput(arguments_list);
   FrameStateInfo state_info = OpParameter<FrameStateInfo>(frame_state);
   int start_index = 0;
@@ skipping 456 matching lines @@
   return jsgraph()->javascript();
 }
 
 SimplifiedOperatorBuilder* JSCallReducer::simplified() const {
   return jsgraph()->simplified();
 }
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8