[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->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.
   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());
+  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.
   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->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) {
   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);
 
-  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());
 
     // 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) {
   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) {
   if (expr->is_jsruntime()) {
     RegisterList args = register_allocator()->NewGrowableRegisterList();
     // Allocate a register for the receiver and load it with undefined.
     BuildPushUndefinedIntoRegisterList(&args);
     VisitArguments(expr->arguments(), &args);
     builder()->CallJSRuntime(expr->context_index(), args);
   } else {
@@ skipping 727 matching lines @@
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8