Move desugaring of super calls with trailing spread to one runtime call.

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 1200cacd0707baeb4b9dbaf7ffbb079fc94479ad..c8e95faea2ecc79aec2f3cc0b983365e417ace2c 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -2486,24 +2486,68 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
   Register constructor = this_function;  // Re-use dead this_function register.
   builder()->StoreAccumulatorInRegister(constructor);
 
-  RegisterList args =
-      register_allocator()->NewRegisterList(expr->arguments()->length());
-  VisitArguments(expr->arguments(), args);
+  ZoneList<Expression*>* args = expr->arguments();
+  RegisterList args_regs =
+      register_allocator()->NewRegisterList(args->length());

[rmcilroy, 2016/11/29 15:51:56]

These are only used in the else branch, please move them there.

[petermarshall, 2016/11/30 13:11:09]

Done

+
+  bool has_spread = false;
+  for (int i = 0; i < args->length(); i++) {
+    if (args->at(i)->IsSpread()) has_spread = true;
+  }
+
+  if (has_spread) {
+    // Pre-allocate regs for ReflectConstruct so we can use them.
+    RegisterList reflect_construct_args =
+        register_allocator()->NewRegisterList(4);

[rmcilroy, 2016/11/29 15:51:56]

You can move this down to the first point you use it (above
builder()->StoreAccumulatorInRegister(reflect_construct_args[2]) - this will
avoid keeping these registers allocated while visiting the arguments.

[petermarshall, 2016/11/30 13:11:09]

Done

+
+    // Deal with the spread arg

[rmcilroy, 2016/11/29 15:51:56]

fullstop on comments

[petermarshall, 2016/11/30 13:11:09]

Done

+    if (args->length() == 1) {
+      Register spread_array = VisitForRegisterValue(
+          args->at(args->length() - 1)->AsSpread()->expression());
+      Runtime::FunctionId function_id = Runtime::kSpreadIterablePrepare;
+      builder()->CallRuntime(function_id, spread_array);
+    } else {
+      RegisterList spread_prepare_args =
+          register_allocator()->NewRegisterList(args->length());
+      // visit args at 0 --> length -2 inclusive

[rmcilroy, 2016/11/29 15:51:56]

And capitals

[rmcilroy, 2016/11/29 15:51:56]

length -2 -> length - 2 

[petermarshall, 2016/11/30 13:11:09]

Done

+      for (int i = 0; i < args->length() - 1; i++) {
+        VisitForRegisterValue(args->at(i), spread_prepare_args[i]);
+      }
+      // visit the spread
+      VisitForRegisterValue(
+          args->at(args->length() - 1)->AsSpread()->expression(),
+          spread_prepare_args[args->length() - 1]);

[rmcilroy, 2016/11/29 15:51:56]

Is this not assuming the spread arg is at the end of the arguments? If this is
the case, why do we need to loop through all the arguments to check if there is
a spread at line 2594?

[petermarshall, 2016/11/30 13:11:09]

There are two cases here - with and without spread. With spread must call into
Reflect.Construct, without can just emit the New bytecode with the args it was
passed.

It gets kind of confusing because we only create SuperCall nodes if the spread
is the last argument. So a spread being present implies that it is the last
argument.

[rmcilroy, 2016/11/30 14:35:32]

Right, My point was more that if the spread is only present on the last argument
(if it exists) means the for loop on line 2494 is unnecessary, just check check:
  if (args->at(args->length() - 1)->IsSpread() {
    ... Do REFLECT_CONSTRUCT_INDEX code
  } else {
    // DCHECK none of the other arguments are spread (if necessary)
    .. Do normal new case 
  }

[petermarshall, 2016/11/30 15:24:48]

Ahhhh I see your point, that is much nicer. Need to check that args isn't empty
too. Also there is a handy last() function.

 
-  // The new target is loaded into the accumulator from the
-  // {new.target} variable.
-  VisitForAccumulatorValue(super->new_target_var());
+      Runtime::FunctionId function_id = Runtime::kSpreadIterablePrepareVarargs;
+      builder()->CallRuntime(function_id, spread_prepare_args);
+    }

[rmcilroy, 2016/11/29 15:51:56]

This seems a lot of special casing for either 1 spread argument or multiple
spreads - can't the runtime function just deal with the case were you only have
one spread arg or multiple instead of doing something different for both cases?

[petermarshall, 2016/11/30 13:11:09]

Yeah that is definitely much simpler, done.

[rmcilroy, 2016/11/29 15:51:56]

Could the code dealing with spread arguments be moved to a seperate function
(e.g., VisitSpreadArguments or similar)?

[petermarshall, 2016/11/30 13:11:09]

Yep much simpler, done

-  // Call construct.
-  builder()->SetExpressionPosition(expr);
-  // TODO(turbofan): For now we do gather feedback on super constructor
-  // calls, utilizing the existing machinery to inline the actual call
-  // target and the JSCreate for the implicit receiver allocation. This
-  // is not an ideal solution for super constructor calls, but it gets
-  // the job done for now. In the long run we might want to revisit this
-  // and come up with a better way.
-  int const feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());
-  builder()->New(constructor, args, feedback_slot_index);
+    builder()->StoreAccumulatorInRegister(reflect_construct_args[2]);
+
+    // Do ReflectConstruct
+    Register receiver = reflect_construct_args[0];
+    builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
+    builder()->MoveRegister(constructor, reflect_construct_args[1]);
+    VisitForRegisterValue(super->new_target_var(), reflect_construct_args[3]);
+    builder()->CallJSRuntime(Context::REFLECT_CONSTRUCT_INDEX,
+                             reflect_construct_args);

[rmcilroy, 2016/11/29 15:51:56]

Please add tests for this in test-bytecode-generator for bytecode expectations.

[petermarshall, 2016/11/30 13:11:09]

Done

+  } else {
+    VisitArguments(args, args_regs);
+    // The new target is loaded into the accumulator from the
+    // {new.target} variable.
+    VisitForAccumulatorValue(super->new_target_var());
+
+    // Call construct.
+    builder()->SetExpressionPosition(expr);
+    // TODO(turbofan): For now we do gather feedback on super constructor
+    // calls, utilizing the existing machinery to inline the actual call
+    // target and the JSCreate for the implicit receiver allocation. This
+    // is not an ideal solution for super constructor calls, but it gets
+    // the job done for now. In the long run we might want to revisit this
+    // and come up with a better way.
+    int const feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());
+    builder()->New(constructor, args_regs, feedback_slot_index);
+  }
 }
 
 void BytecodeGenerator::VisitCallNew(CallNew* expr) {