[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..d3f649afa050e3f25725c4d9379cd9f06445b3a6 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -2415,12 +2415,19 @@ 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->arguments()->is_empty() && expr->arguments()->last()->IsSpread()) {

[rmcilroy, 2017/01/16 10:29:40]

Could you pull this out to a boolean and reuse it here and below, or even
better, make it an attribute on the Call Expression?

[petermarshall, 2017/01/16 15:01:10]

Yes I like it on the Call Expression (and CallNew), done.

+    args = register_allocator()->NewGrowableRegisterList();
+    callee = register_allocator()->GrowRegisterList(&args);
+  } else {
+    callee = register_allocator()->NewRegister();
+    args = register_allocator()->NewGrowableRegisterList();
+  }
 
   // Prepare the callee and the receiver to the function call. This depends on
   // the semantics of the underlying call type.
@@ -2429,7 +2436,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 +2497,6 @@ 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());

[rmcilroy, 2017/01/16 10:29:40]

Please leave this DCHECK in for non spread calls (and it should work for spread
calls after you change the bytecode like NewWithSpread)

[petermarshall, 2017/01/16 15:01:10]

Yep, Done.

 
   // Resolve callee for a potential direct eval call. This block will mutate the
   // callee value.
@@ -2520,9 +2526,18 @@ 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->arguments()->is_empty() && expr->arguments()->last()->IsSpread()) {
+    CHECK_EQ(expr->arguments()->length() + 2, args.register_count());
+    DCHECK_EQ(expr->tail_call_mode(), TailCallMode::kDisallow);

[Benedikt Meurer, 2017/01/14 18:37:14]

Nit: DCHECK_EQ(TailCallMode::kDisallow, ...)

[petermarshall, 2017/01/16 15:01:10]

Done.

+    builder()->CallWithSpread(args);

[rmcilroy, 2017/01/16 10:29:40]

The issue with this is that we already have a CallProperty call for named /
keyed property calls (decided via the call_type passed to builder()->Call(..)
below. Now we no longer get the information about whether a spread is a property
call or not. Personally I'm thinking we have far too many call bytecodes all of
which do mostly the same thing. Once CallWithSpread is more similar to the Call
bytecode (calling the InterpreterPushArgs, etc). Could we think about adding a
seperate argument to call which specifies the call type (e.g., property, spread,
tailcall, etc.) and only have a single handler for call. WDYT? Not necessary for
this CL, but please add a TODO if you think it's possible.

[petermarshall, 2017/01/16 15:01:10]

Yeah I think that suggestion makes sense - added a TODO higher up in this
function.

+  } else {
+    CHECK_EQ(expr->arguments()->length() + 1, args.register_count());
+    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) {