Reland "[parsing] inline ArrayLiteral creation for spread calls"

This reverts commit f48e7349.

Reason for revert: innocent!!

Original change's description:
> Revert "[parsing] inline ArrayLiteral creation for spread calls"
> 
> This reverts commit 93fc3841.
> 
> Reason for revert: may break node.js integration
> 
> Original change's description:
> > [parsing] inline ArrayLiteral creation for spread calls
> > 
> > Instead of using runtime calls to generate the Array Literal passed to
> > %reflect_call / %reflect_construct, we create an ArrayLiteral from the
> > list of arguments, and perform spreads using the interpreter mechanism for
> > spreading in ArrayLiterals (thus, the spreading becomes inline). This
> > array literal is still passed to %reflect_call / %reflect_construct as
> > before.
> > 
> > This cuts the runtime for bench-spread-call.js -> testSpread roughly in
> > half, and will likely improve further once
> > https://chromium-review.googlesource.com/c/v8/v8/+/915364 has landed.
> > 
> > BUG=v8:7446
> > R=​neis@chromium.org, adamk@chromium.org
> > 
> > Change-Id: I74a6acd3a60aad422e4ac575275c7b567659d8ad
> > Reviewed-on: https://chromium-review.googlesource.com/939587
> > Commit-Queue: Georg Neis <neis@chromium.org>
> > Reviewed-by: Georg Neis <neis@chromium.org>
> > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#51678}
> 
> TBR=adamk@chromium.org,neis@chromium.org,caitp@igalia.com,bmeurer@chromium.org
> 
> Change-Id: I4730077591bce0e5e7b2ce7d59678e8b7135cc08
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: v8:7446
> Reviewed-on: https://chromium-review.googlesource.com/945769
> Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
> Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#51682}

TBR=adamk@chromium.org,neis@chromium.org,sigurds@chromium.org,caitp@igalia.com,bmeurer@chromium.org

Change-Id: I977513bea06a4f3fba03fa4a89270298475422e2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7446
Reviewed-on: https://chromium-review.googlesource.com/945808Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51686}

src/contexts.h

@@ -57,7 +57,6 @@ enum ContextLookupFlags {
   V(REFLECT_CONSTRUCT_INDEX, JSFunction, reflect_construct)             \
   V(REFLECT_DEFINE_PROPERTY_INDEX, JSFunction, reflect_define_property) \
   V(REFLECT_DELETE_PROPERTY_INDEX, JSFunction, reflect_delete_property) \
-  V(SPREAD_ARGUMENTS_INDEX, JSFunction, spread_arguments)               \
   V(SPREAD_ITERABLE_INDEX, JSFunction, spread_iterable)                 \
   V(MATH_FLOOR_INDEX, JSFunction, math_floor)                           \
   V(MATH_POW_INDEX, JSFunction, math_pow)                               \

src/js/spread.js

@@ -12,22 +12,6 @@ var InternalArray = utils.InternalArray;
 
 // -------------------------------------------------------------------
 
-function SpreadArguments() {
-  var count = arguments.length;
-  var args = new InternalArray();
-
-  for (var i = 0; i < count; ++i) {
-    var array = arguments[i];
-    var length = array.length;
-    for (var j = 0; j < length; ++j) {
-      args.push(array[j]);
-    }
-  }
-
-  return args;
-}
-
-
 function SpreadIterable(collection) {
   if (IS_NULL_OR_UNDEFINED(collection)) {
     throw %make_type_error(kNotIterable, collection);
@@ -44,7 +28,6 @@ function SpreadIterable(collection) {
 // Exports
 
 %InstallToContext([
-  "spread_arguments", SpreadArguments,
   "spread_iterable", SpreadIterable,
 ]);
 

src/parsing/parser.cc

@@ -3564,96 +3564,52 @@ bool OnlyLastArgIsSpread(ZoneList<Expression*>* args) {
 
 }  // namespace
 
-ZoneList<Expression*>* Parser::PrepareSpreadArguments(
+ArrayLiteral* Parser::ArrayLiteralFromListWithSpread(
     ZoneList<Expression*>* list) {
-  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
-  if (list->length() == 1) {
-    // Spread-call with single spread argument produces an InternalArray
-    // containing the values from the array.
-    //
-    // Function is called or constructed with the produced array of arguments
-    //
-    // EG: Apply(Func, Spread(spread0))
-    ZoneList<Expression*>* spread_list =
-        new (zone()) ZoneList<Expression*>(0, zone());
-    spread_list->Add(list->at(0)->AsSpread()->expression(), zone());
-    args->Add(factory()->NewCallRuntime(Runtime::kSpreadIterablePrepare,
-                                        spread_list, kNoSourcePosition),
-              zone());
-    return args;
-  } else {
-    // Spread-call with multiple arguments produces array literals for each
-    // sequences of unspread arguments, and converts each spread iterable to
-    // an Internal array. Finally, all of these produced arrays are flattened
-    // into a single InternalArray, containing the arguments for the call.
-    //
-    // EG: Apply(Func, Flatten([unspread0, unspread1], Spread(spread0),
-    //                         Spread(spread1), [unspread2, unspread3]))
-    int i = 0;
-    int n = list->length();
-    while (i < n) {
-      if (!list->at(i)->IsSpread()) {
-        ZoneList<Expression*>* unspread =
-            new (zone()) ZoneList<Expression*>(1, zone());
-
-        // Push array of unspread parameters
-        while (i < n && !list->at(i)->IsSpread()) {
-          unspread->Add(list->at(i++), zone());
-        }
-        args->Add(factory()->NewArrayLiteral(unspread, kNoSourcePosition),
-                  zone());
-
-        if (i == n) break;
-      }
+  // If there's only a single spread argument, a fast path using CallWithSpread
+  // is taken.
+  DCHECK_LT(1, list->length());
 
-      // Push eagerly spread argument
-      ZoneList<Expression*>* spread_list =
-          new (zone()) ZoneList<Expression*>(1, zone());
-      spread_list->Add(list->at(i++)->AsSpread()->expression(), zone());
-      args->Add(factory()->NewCallRuntime(Context::SPREAD_ITERABLE_INDEX,
-                                          spread_list, kNoSourcePosition),
-                zone());
-    }
-
-    list = new (zone()) ZoneList<Expression*>(1, zone());
-    list->Add(factory()->NewCallRuntime(Context::SPREAD_ARGUMENTS_INDEX, args,
-                                        kNoSourcePosition),
-              zone());
-    return list;
+  // The arguments of the spread call become a single ArrayLiteral.
+  int first_spread = 0;
+  for (; first_spread < list->length() && !list->at(first_spread)->IsSpread();
+       ++first_spread) {
   }
-  UNREACHABLE();
+
+  DCHECK_LT(first_spread, list->length());
+  return factory()->NewArrayLiteral(list, first_spread, kNoSourcePosition);
 }
 
 Expression* Parser::SpreadCall(Expression* function,
-                               ZoneList<Expression*>* args, int pos,
+                               ZoneList<Expression*>* args_list, int pos,
                                Call::PossiblyEval is_possibly_eval) {
   // Handle this case in BytecodeGenerator.
-  if (OnlyLastArgIsSpread(args)) {
-    return factory()->NewCall(function, args, pos);
+  if (OnlyLastArgIsSpread(args_list)) {
+    return factory()->NewCall(function, args_list, pos);
   }
 
   if (function->IsSuperCallReference()) {
     // Super calls
     // $super_constructor = %_GetSuperConstructor(<this-function>)
     // %reflect_construct($super_constructor, args, new.target)
-
-    args = PrepareSpreadArguments(args);
+    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone());
     ZoneList<Expression*>* tmp = new (zone()) ZoneList<Expression*>(1, zone());
     tmp->Add(function->AsSuperCallReference()->this_function_var(), zone());
-    Expression* super_constructor = factory()->NewCallRuntime(
-        Runtime::kInlineGetSuperConstructor, tmp, pos);
-    args->InsertAt(0, super_constructor, zone());
+    args->Add(factory()->NewCallRuntime(Runtime::kInlineGetSuperConstructor,
+                                        tmp, pos),
+              zone());
+    args->Add(ArrayLiteralFromListWithSpread(args_list), zone());
     args->Add(function->AsSuperCallReference()->new_target_var(), zone());
     return factory()->NewCallRuntime(Context::REFLECT_CONSTRUCT_INDEX, args,
                                      pos);
   } else {
-    args = PrepareSpreadArguments(args);
+    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone());
     if (function->IsProperty()) {
       // Method calls
       if (function->AsProperty()->IsSuperAccess()) {
         Expression* home = ThisExpression(kNoSourcePosition);
-        args->InsertAt(0, function, zone());
-        args->InsertAt(1, home, zone());
+        args->Add(function, zone());
+        args->Add(home, zone());
       } else {
         Variable* temp = NewTemporary(ast_value_factory()->empty_string());
         VariableProxy* obj = factory()->NewVariableProxy(temp);
@@ -3662,28 +3618,29 @@ Expression* Parser::SpreadCall(Expression* function,
             kNoSourcePosition);
         function = factory()->NewProperty(
             assign_obj, function->AsProperty()->key(), kNoSourcePosition);
-        args->InsertAt(0, function, zone());
+        args->Add(function, zone());
         obj = factory()->NewVariableProxy(temp);
-        args->InsertAt(1, obj, zone());
+        args->Add(obj, zone());
       }
     } else {
       // Non-method calls
-      args->InsertAt(0, function, zone());
-      args->InsertAt(1, factory()->NewUndefinedLiteral(kNoSourcePosition),
-                     zone());
+      args->Add(function, zone());
+      args->Add(factory()->NewUndefinedLiteral(kNoSourcePosition), zone());
     }
+    args->Add(ArrayLiteralFromListWithSpread(args_list), zone());
     return factory()->NewCallRuntime(Context::REFLECT_APPLY_INDEX, args, pos);
   }
 }
 
 Expression* Parser::SpreadCallNew(Expression* function,
-                                  ZoneList<Expression*>* args, int pos) {
-  if (OnlyLastArgIsSpread(args)) {
+                                  ZoneList<Expression*>* args_list, int pos) {
+  if (OnlyLastArgIsSpread(args_list)) {
     // Handle in BytecodeGenerator.
-    return factory()->NewCallNew(function, args, pos);
+    return factory()->NewCallNew(function, args_list, pos);
   }
-  args = PrepareSpreadArguments(args);
-  args->InsertAt(0, function, zone());
+  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
+  args->Add(function, zone());
+  args->Add(ArrayLiteralFromListWithSpread(args_list), zone());
 
   return factory()->NewCallRuntime(Context::REFLECT_CONSTRUCT_INDEX, args, pos);
 }

src/parsing/parser.h

@@ -502,7 +502,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   Expression* CloseTemplateLiteral(TemplateLiteralState* state, int start,
                                    Expression* tag);
 
-  ZoneList<Expression*>* PrepareSpreadArguments(ZoneList<Expression*>* list);
+  ArrayLiteral* ArrayLiteralFromListWithSpread(ZoneList<Expression*>* list);
   Expression* SpreadCall(Expression* function, ZoneList<Expression*>* args,
                          int pos, Call::PossiblyEval is_possibly_eval);
   Expression* SpreadCallNew(Expression* function, ZoneList<Expression*>* args,

src/runtime/runtime-array.cc

@@ -794,23 +794,5 @@ RUNTIME_FUNCTION(Runtime_ArrayIndexOf) {
   return Smi::FromInt(-1);
 }
 
-
-RUNTIME_FUNCTION(Runtime_SpreadIterablePrepare) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(1, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(Object, spread, 0);
-
-  // Iterate over the spread if we need to.
-  if (spread->IterationHasObservableEffects()) {
-    Handle<JSFunction> spread_iterable_function = isolate->spread_iterable();
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, spread,
-        Execution::Call(isolate, spread_iterable_function,
-                        isolate->factory()->undefined_value(), 1, &spread));
-  }
-
-  return *spread;
-}
-
 }  // namespace internal
 }  // namespace v8

src/runtime/runtime.h

@@ -51,8 +51,7 @@ namespace internal {
   F(ArrayIsArray, 1, 1)                  \
   F(ArraySpeciesConstructor, 1, 1)       \
   F(ArrayIncludes_Slow, 3, 1)            \
-  F(ArrayIndexOf, 3, 1)                  \
-  F(SpreadIterablePrepare, 1, 1)
+  F(ArrayIndexOf, 3, 1)
 
 #define FOR_EACH_INTRINSIC_ATOMICS(F)           \
   F(AtomicsExchange, 3, 1)                      \

test/cctest/compiler/test-run-bytecode-graph-builder.cc

@@ -618,7 +618,7 @@ TEST(BytecodeGraphBuilderCallRuntime) {
        {factory->true_value(), BytecodeGraphTester::NewObject("[1, 2, 3]")}},
       {"function f(arg0) { return %Add(arg0, 2) }\nf(1)",
        {factory->NewNumberFromInt(5), factory->NewNumberFromInt(3)}},
-      {"function f(arg0) { return %spread_arguments(arg0).length }\nf([])",
+      {"function f(arg0) { return %spread_iterable(arg0).length }\nf([])",
        {factory->NewNumberFromInt(3),
         BytecodeGraphTester::NewObject("[1, 2, 3]")}},
   };

test/cctest/interpreter/bytecode_expectations/CallAndSpread.golden

@@ -65,9 +65,9 @@ handlers: [
 snippet: "
   Math.max(0, ...[1, 2, 3], 4);
 "
-frame size: 6
+frame size: 11
 parameter count: 1
-bytecode array length: 51
+bytecode array length: 109
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   34 S> */ B(LdaGlobal), U8(0), U8(0),
@@ -75,16 +75,34 @@ bytecodes: [
                 B(LdaNamedProperty), R(0), U8(1), U8(2),
                 B(Star), R(1),
                 B(CreateArrayLiteral), U8(2), U8(4), U8(37),
-                B(Star), R(3),
-                B(CreateArrayLiteral), U8(3), U8(5), U8(37),
-                B(Star), R(4),
-                B(CallJSRuntime), U8(%spread_iterable), R(4), U8(1),
                 B(Star), R(4),
-                B(CreateArrayLiteral), U8(4), U8(6), U8(37),
-                B(Star), R(5),
-                B(CallJSRuntime), U8(%spread_arguments), R(3), U8(3),
-                B(Star), R(3),
+  /*   49 S> */ B(CreateArrayLiteral), U8(3), U8(5), U8(37),
+                B(Star), R(9),
+                B(LdaNamedProperty), R(9), U8(4), U8(6),
+                B(Star), R(10),
+                B(CallProperty0), R(10), R(9), U8(8),
                 B(Mov), R(0), R(2),
+                B(Mov), R(4), R(5),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
+                B(Star), R(8),
+                B(LdaNamedProperty), R(8), U8(5), U8(10),
+                B(Star), R(7),
+                B(CallProperty0), R(7), R(8), U8(12),
+                B(Star), R(6),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(6), U8(1),
+                B(LdaNamedProperty), R(6), U8(6), U8(14),
+                B(JumpIfToBooleanTrue), U8(16),
+                B(LdaNamedProperty), R(6), U8(7), U8(16),
+                B(Star), R(6),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(5), U8(2),
+                B(JumpLoop), U8(30), I8(0),
+                B(LdaSmi), I8(4),
+                B(Star), R(6),
+                B(Mov), R(4), R(5),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(5), U8(2),
+                B(Mov), R(5), R(3),
                 B(CallJSRuntime), U8(%reflect_apply), R(1), U8(3),
                 B(LdaUndefined),
   /*   64 S> */ B(Return),
@@ -94,7 +112,10 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["max"],
   TUPLE2_TYPE,
   TUPLE2_TYPE,
-  TUPLE2_TYPE,
+  SYMBOL_TYPE,
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["next"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["done"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
 ]
 handlers: [
 ]

test/cctest/interpreter/bytecode_expectations/NewAndSpread.golden

@@ -84,9 +84,9 @@ snippet: "
   class A { constructor(...args) { this.args = args; } }
   new A(0, ...[1, 2, 3], 4);
 "
-frame size: 6
+frame size: 11
 parameter count: 1
-bytecode array length: 66
+bytecode array length: 124
 bytecodes: [
   /*   30 E> */ B(StackCheck),
                 B(LdaTheHole),
@@ -101,15 +101,33 @@ bytecodes: [
                 B(Mov), R(4), R(0),
                 B(Mov), R(0), R(1),
   /*   89 S> */ B(CreateArrayLiteral), U8(2), U8(1), U8(37),
-                B(Star), R(3),
-                B(CreateArrayLiteral), U8(3), U8(2), U8(37),
-                B(Star), R(4),
-                B(CallJSRuntime), U8(%spread_iterable), R(4), U8(1),
                 B(Star), R(4),
-                B(CreateArrayLiteral), U8(4), U8(3), U8(37),
-                B(Star), R(5),
-                B(CallJSRuntime), U8(%spread_arguments), R(3), U8(3),
-                B(Star), R(3),
+  /*  101 S> */ B(CreateArrayLiteral), U8(3), U8(2), U8(37),
+                B(Star), R(9),
+                B(LdaNamedProperty), R(9), U8(4), U8(3),
+                B(Star), R(10),
+                B(CallProperty0), R(10), R(9), U8(5),
+                B(Mov), R(4), R(5),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
+                B(Star), R(8),
+                B(LdaNamedProperty), R(8), U8(5), U8(7),
+                B(Star), R(7),
+                B(CallProperty0), R(7), R(8), U8(9),
+                B(Star), R(6),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(6), U8(1),
+                B(LdaNamedProperty), R(6), U8(6), U8(11),
+                B(JumpIfToBooleanTrue), U8(16),
+                B(LdaNamedProperty), R(6), U8(7), U8(13),
+                B(Star), R(6),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(5), U8(2),
+                B(JumpLoop), U8(30), I8(0),
+                B(LdaSmi), I8(4),
+                B(Star), R(6),
+                B(Mov), R(4), R(5),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(5), U8(2),
+                B(Mov), R(5), R(3),
                 B(CallJSRuntime), U8(%reflect_construct), R(2), U8(2),
                 B(LdaUndefined),
   /*  116 S> */ B(Return),
@@ -119,7 +137,10 @@ constant pool: [
   SHARED_FUNCTION_INFO_TYPE,
   TUPLE2_TYPE,
   TUPLE2_TYPE,
-  TUPLE2_TYPE,
+  SYMBOL_TYPE,
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["next"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["done"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
 ]
 handlers: [
 ]

test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden

@@ -91,9 +91,9 @@ snippet: "
     test = new B(1, 2, 3).constructor;
   })();
 "
-frame size: 8
+frame size: 13
 parameter count: 1
-bytecode array length: 60
+bytecode array length: 121
 bytecodes: [
                 B(CreateRestParameter),
                 B(Star), R(2),
@@ -103,13 +103,32 @@ bytecodes: [
   /*  140 S> */ B(CallRuntime), U16(Runtime::k_GetSuperConstructor), R(closure), U8(1),
                 B(Star), R(4),
                 B(CreateArrayLiteral), U8(0), U8(0), U8(37),
-                B(Star), R(5),
-  /*  152 E> */ B(CallJSRuntime), U8(%spread_iterable), R(2), U8(1),
-                B(Star), R(6),
-                B(CreateArrayLiteral), U8(1), U8(1), U8(37),
-                B(Star), R(7),
-                B(CallJSRuntime), U8(%spread_arguments), R(5), U8(3),
-                B(Star), R(5),
+  /*  152 S> */ B(Star), R(6),
+                B(LdaNamedProperty), R(2), U8(1), U8(1),
+                B(Star), R(12),
+                B(CallProperty0), R(12), R(2), U8(3),
+                B(Mov), R(2), R(11),
+                B(Mov), R(6), R(7),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
+                B(Star), R(10),
+                B(LdaNamedProperty), R(10), U8(2), U8(5),
+                B(Star), R(9),
+                B(CallProperty0), R(9), R(10), U8(7),
+                B(Star), R(8),
+                B(JumpIfJSReceiver), U8(7),
+                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(8), U8(1),
+                B(LdaNamedProperty), R(8), U8(3), U8(9),
+                B(JumpIfToBooleanTrue), U8(16),
+                B(LdaNamedProperty), R(8), U8(4), U8(11),
+                B(Star), R(8),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(7), U8(2),
+                B(JumpLoop), U8(30), I8(0),
+                B(LdaSmi), I8(1),
+                B(Star), R(8),
+                B(Mov), R(6), R(7),
+                B(CallRuntime), U16(Runtime::kAppendElement), R(7), U8(2),
+                B(Mov), R(7), R(5),
                 B(Mov), R(0), R(6),
   /*  140 E> */ B(CallJSRuntime), U8(%reflect_construct), R(4), U8(3),
                 B(Star), R(4),
@@ -122,7 +141,10 @@ bytecodes: [
 ]
 constant pool: [
   TUPLE2_TYPE,
-  TUPLE2_TYPE,
+  SYMBOL_TYPE,
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["next"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["done"],
+  ONE_BYTE_INTERNALIZED_STRING_TYPE ["value"],
 ]
 handlers: [
 ]