[Ignition/turbo] Add a CallWithSpread bytecode.

src/interpreter/bytecode-generator.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/interpreter/bytecode-generator.h"
 
 #include "src/ast/compile-time-value.h"
 #include "src/ast/scopes.h"
 #include "src/builtins/builtins-constructor.h"
 #include "src/code-stubs.h"
@@ skipping 2397 matching lines @@
 }
 
 void BytecodeGenerator::VisitCall(Call* expr) {
   Expression* callee_expr = expr->expression();
   Call::CallType call_type = expr->GetCallType();
 
   if (call_type == Call::SUPER_CALL) {
     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.
   switch (call_type) {
     case Call::NAMED_PROPERTY_CALL:
     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: {
       // Receiver is undefined for global calls.
       BuildPushUndefinedIntoRegisterList(&args);
       // Load callee as a global variable.
       VariableProxy* proxy = callee_expr->AsVariableProxy();
       BuildVariableLoadForAccumulatorValue(proxy->var(),
                                            proxy->VariableFeedbackSlot(),
                                            proxy->hole_check_mode());
@@ skipping 40 matching lines @@
       break;
     }
     case Call::SUPER_CALL:
       UNREACHABLE();
       break;
   }
 
   // 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.
   if (expr->is_possibly_eval() && expr->arguments()->length() > 0) {
     RegisterAllocationScope inner_register_scope(this);
     // Set up arguments for ResolvePossiblyDirectEval by copying callee, source
     // strings and function closure, and loading language and
     // position.
     RegisterList runtime_call_args = register_allocator()->NewRegisterList(6);
     builder()
         ->MoveRegister(callee, runtime_call_args[0])
         .MoveRegister(args[1], runtime_call_args[1])
         .MoveRegister(Register::function_closure(), runtime_call_args[2])
         .LoadLiteral(Smi::FromInt(language_mode()))
         .StoreAccumulatorInRegister(runtime_call_args[3])
         .LoadLiteral(
             Smi::FromInt(execution_context()->scope()->start_position()))
         .StoreAccumulatorInRegister(runtime_call_args[4])
         .LoadLiteral(Smi::FromInt(expr->position()))
         .StoreAccumulatorInRegister(runtime_call_args[5]);
 
     // Call ResolvePossiblyDirectEval and modify the callee.
     builder()
         ->CallRuntime(Runtime::kResolvePossiblyDirectEval, runtime_call_args)
         .StoreAccumulatorInRegister(callee);
   }
 
   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) {
   RegisterAllocationScope register_scope(this);
   SuperCallReference* super = expr->expression()->AsSuperCallReference();
 
   // Prepare the constructor to the super call.
   VisitForAccumulatorValue(super->this_function_var());
   Register constructor = register_allocator()->NewRegister();
   builder()->GetSuperConstructor(constructor);
@@ skipping 785 matching lines @@
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8