[Ignition/turbo] Add a CallWithSpread bytecode.

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 7279f69d8a01c554e1b0b627f423c994331568f8..7157ef638d608870d73c1b315640b2d67e4e3341 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -2415,12 +2415,23 @@ void BytecodeGenerator::VisitCall(Call* expr) {
     return VisitCallSuper(expr);
   }
 
-  Register callee = register_allocator()->NewRegister();
   // Grow the args list as we visit receiver / arguments to avoid allocating all
   // the registers up-front. Otherwise these registers are unavailable during
   // receiver / argument visiting and we can end up with memory leaks due to
   // registers keeping objects alive.
-  RegisterList args = register_allocator()->NewGrowableRegisterList();
+  RegisterList args;
+  Register callee;
+  if (expr->only_last_arg_is_spread()) {
+    args = register_allocator()->NewGrowableRegisterList();

[rmcilroy, 2017/01/16 16:04:35]

nit - could you add a comment on why this is different for spread and a TODO to
remove this difference once the bytecode is changed.

[petermarshall, 2017/01/17 08:39:38]

Done

+    callee = register_allocator()->GrowRegisterList(&args);
+  } else {
+    callee = register_allocator()->NewRegister();
+    args = register_allocator()->NewGrowableRegisterList();
+  }
+
+  // TODO(petermarshall): We have a lot of call bytecodes that are very similar,
+  // see if we can reduce the number by adding a separate argument which
+  // specifies the call type (e.g., property, spread, tailcall, etc.).
 
   // Prepare the callee and the receiver to the function call. This depends on
   // the semantics of the underlying call type.
@@ -2429,7 +2440,7 @@ void BytecodeGenerator::VisitCall(Call* expr) {
     case Call::KEYED_PROPERTY_CALL: {
       Property* property = callee_expr->AsProperty();
       VisitAndPushIntoRegisterList(property->obj(), &args);
-      VisitPropertyLoadForRegister(args[0], property, callee);
+      VisitPropertyLoadForRegister(args.last_register(), property, callee);
       break;
     }
     case Call::GLOBAL_CALL: {
@@ -2490,7 +2501,9 @@ void BytecodeGenerator::VisitCall(Call* expr) {
   // Evaluate all arguments to the function call and store in sequential args
   // registers.
   VisitArguments(expr->arguments(), &args);
-  CHECK_EQ(expr->arguments()->length() + 1, args.register_count());
+  if (!expr->only_last_arg_is_spread()) {
+    CHECK_EQ(expr->arguments()->length() + 1, args.register_count());

[rmcilroy, 2017/01/16 16:04:35]

nit - please add a TODO to check this for spread calls when the bytecode
changes.

[petermarshall, 2017/01/17 08:39:39]

Done

+  }
 
   // Resolve callee for a potential direct eval call. This block will mutate the
   // callee value.
@@ -2520,9 +2533,17 @@ void BytecodeGenerator::VisitCall(Call* expr) {
 
   builder()->SetExpressionPosition(expr);
 
-  int const feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());
-  builder()->Call(callee, args, feedback_slot_index, call_type,
-                  expr->tail_call_mode());
+  // When a call contains a spread, a Call AST node is only created if there is
+  // exactly one spread, and it is the last argument.
+  if (expr->only_last_arg_is_spread()) {
+    CHECK_EQ(expr->arguments()->length() + 2, args.register_count());
+    DCHECK_EQ(TailCallMode::kDisallow, expr->tail_call_mode());
+    builder()->CallWithSpread(args);

[rmcilroy, 2017/01/16 16:04:35]

Please add a test to test-bytecode-generator which exercises this codepath.

[petermarshall, 2017/01/17 08:39:39]

Done

+  } else {
+    int const feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());
+    builder()->Call(callee, args, feedback_slot_index, call_type,
+                    expr->tail_call_mode());
+  }
 }
 
 void BytecodeGenerator::VisitCallSuper(Call* expr) {
@@ -2534,25 +2555,22 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
   Register constructor = register_allocator()->NewRegister();
   builder()->GetSuperConstructor(constructor);
 
-  ZoneList<Expression*>* args = expr->arguments();
-
   // When a super call contains a spread, a CallSuper AST node is only created
   // if there is exactly one spread, and it is the last argument.
-  if (!args->is_empty() && args->last()->IsSpread()) {
-    RegisterList args_regs = register_allocator()->NewGrowableRegisterList();
-    Register constructor_arg =
-        register_allocator()->GrowRegisterList(&args_regs);
+  if (expr->only_last_arg_is_spread()) {
+    RegisterList args = register_allocator()->NewGrowableRegisterList();
+    Register constructor_arg = register_allocator()->GrowRegisterList(&args);
     builder()->MoveRegister(constructor, constructor_arg);
     // Reserve argument reg for new.target in correct place for runtime call.
     // TODO(petermarshall): Remove this when changing bytecode to use the new
     // stub.
-    Register new_target = register_allocator()->GrowRegisterList(&args_regs);
-    VisitArguments(args, &args_regs);
+    Register new_target = register_allocator()->GrowRegisterList(&args);
+    VisitArguments(expr->arguments(), &args);
     VisitForRegisterValue(super->new_target_var(), new_target);
-    builder()->NewWithSpread(args_regs);
+    builder()->NewWithSpread(args);
   } else {
     RegisterList args_regs = register_allocator()->NewGrowableRegisterList();
-    VisitArguments(args, &args_regs);
+    VisitArguments(expr->arguments(), &args_regs);
     // The new target is loaded into the accumulator from the
     // {new.target} variable.
     VisitForAccumulatorValue(super->new_target_var());
@@ -2573,14 +2591,28 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
 void BytecodeGenerator::VisitCallNew(CallNew* expr) {
   Register constructor = VisitForRegisterValue(expr->expression());
   RegisterList args = register_allocator()->NewGrowableRegisterList();
-  VisitArguments(expr->arguments(), &args);
 
-  builder()->SetExpressionPosition(expr);
-  // The accumulator holds new target which is the same as the
-  // constructor for CallNew.
-  builder()
-      ->LoadAccumulatorWithRegister(constructor)
-      .New(constructor, args, feedback_index(expr->CallNewFeedbackSlot()));
+  if (expr->only_last_arg_is_spread()) {
+    Register constructor_arg = register_allocator()->GrowRegisterList(&args);
+    Register new_target = register_allocator()->GrowRegisterList(&args);
+
+    builder()
+        ->MoveRegister(constructor, constructor_arg)
+        .MoveRegister(constructor, new_target);
+    VisitArguments(expr->arguments(), &args);
+
+    builder()->SetExpressionPosition(expr);
+    builder()->NewWithSpread(args);
+  } else {
+    VisitArguments(expr->arguments(), &args);
+
+    builder()->SetExpressionPosition(expr);
+    // The accumulator holds new target which is the same as the
+    // constructor for CallNew.
+    builder()
+        ->LoadAccumulatorWithRegister(constructor)
+        .New(constructor, args, feedback_index(expr->CallNewFeedbackSlot()));
+  }

[rmcilroy, 2017/01/16 16:04:35]

This seems like an unrelated change, could you do it in a separate CL please.

[petermarshall, 2017/01/17 08:39:39]

I thought about that but it is actually quite difficult to split them apart. In
parser-base.h we have to make sure we have the same number of
MaterializedLiterals as the parser. These new bytecodes mean that we don't
materialize any literals for the call/super call arguments as long as we can use
the bytecode, rather than the fallback desugaring in the parser.

Where we do that in the preparser, we don't have the info about whether this is
a super call, a regular call or a call to new, so having different behaviour for
these will break a test that expects the number of materialized literals to
always be the same between preparser and parser. Basically I think it would be
possible but might involve some refactoring or hackery that wouldn't really be
worth it. WDYT?

[rmcilroy, 2017/01/17 10:11:31]

I see, in that case I'm fine with it being here, but please update the CL
description to mention it makes this change too. Could you also add this case to
the test-bytecode-generator tests.

 }
 
 void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {