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

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/code-stubs.h"
 #include "src/compilation-info.h"
@@ skipping 2468 matching lines @@
   RegisterAllocationScope register_scope(this);
   SuperCallReference* super = expr->expression()->AsSuperCallReference();
 
   // Prepare the constructor to the super call.
   Register this_function = VisitForRegisterValue(super->this_function_var());
   builder()->CallRuntime(Runtime::kInlineGetSuperConstructor, this_function);
 
   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

 
-  // The new target is loaded into the accumulator from the
-  // {new.target} variable.
-  VisitForAccumulatorValue(super->new_target_var());
+  bool has_spread = false;
+  for (int i = 0; i < args->length(); i++) {
+    if (args->at(i)->IsSpread()) has_spread = true;
+  }
 
-  // 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);
+  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.

+
+      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

+    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) {
   Register constructor = VisitForRegisterValue(expr->expression());
   RegisterList args =
       register_allocator()->NewRegisterList(expr->arguments()->length());
   VisitArguments(expr->arguments(), args);
 
   builder()->SetExpressionPosition(expr);
   // The accumulator holds new target which is the same as the
@@ skipping 679 matching lines @@
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8