WIP: Implement ES6 Spread-calls

WIP: Implement ES6 Spread-calls

Superceded by https://crrev.com/938443002

BUG=v8:3018
R=
LOG=N

[caitp (gmail), 2015-02-11 16:31:33]

Prototype implementation --- tests are missing method spread-calls (because they
were failing pretty badly), but they worked okay testing in d8.

[caitp (gmail), 2015-02-11 16:33:07]

Patchset #1 (id:1) has been deleted

[caitp (gmail), 2015-02-12 19:04:35]

Patchset #1 (id:20001) has been deleted

[arv (Not doing code reviews), 2015-02-12 21:41:33]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-02-12 21:41:34]

Looks like a good start

https://codereview.chromium.org/919703003/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/919703003/diff/40001/src/ast.h#newcode1921
src/ast.h:1921: HasSpreadArgumentsField::encode(HasSpreadArguments(arguments)))
{
Maybe have the parser keep track of this and set the flag instead?

https://codereview.chromium.org/919703003/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/919703003/diff/40001/src/preparser.h#newcode2301
src/preparser.h:2301: bool is_spread = peek() == Token::ELLIPSIS;
needs feature flag

https://codereview.chromium.org/919703003/diff/40001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/919703003/diff/40001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:5155:
VisitForAccumulatorValue(expr->result_value());
Why not VisitForStackValue?

https://codereview.chromium.org/919703003/diff/40001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/919703003/diff/40001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:4925: "......[1,2,3]",
maybe a test for ... outside Arguments?

[caitp (gmail), 2015-02-12 21:58:54]

I was going to add a flag after doing a bit more work prototyping it, but it's
just simpler to work on it without the flag for now imho. Some things need to be
fixed and figured out first

https://codereview.chromium.org/919703003/diff/40001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/919703003/diff/40001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:5155:
VisitForAccumulatorValue(expr->result_value());
On 2015/02/12 21:41:34, arv wrote:
> Why not VisitForStackValue?

right now, it's sort of like this:

before first value:
%rax -> ???
| <arg count from register> | ...

after getting value
| <arg count from register> | <arg count from previous stack value + 1>

after getting value and moving value to stack:
| iterator value            | <arg count>

after iterating, the stack has all the iterated values, and the arg count on the
end, so you just pop the arg count into the arg_count register. It seems to work
okay (but potentially breaks depending on what non-spread arguments are pushed,
though I haven't been able to find one that broke it).

I posted on v8-team-harmony asking about suggested ways to do this, because I'm
having trouble finding a register which won't get clobbered, so preserving the
count on the stack seems to be the only thing I can do. Suggestions are welcome!

https://codereview.chromium.org/919703003/diff/40001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/919703003/diff/40001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:4925: "......[1,2,3]",
On 2015/02/12 21:41:34, arv wrote:
> maybe a test for ... outside Arguments?

will do

[Michael Starzinger, 2015-02-16 19:58:57]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-02-16 19:58:58]

I have questions ...

https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2236: VisitForValue(expr->result_value());
I am not sure how this is supposed to work in the end. But if I understand
correctly then the operand stack size in full-codegen is not statically known
within the loop, because each iteration over the loop pushes one element in the
operand stack.

In TurboFan (as well as in Crankshaft for that matter) the environment is a
static simulation of the operand stack (similar to an abstract interpretation).
But here the operand stack cannot be simulated.

[Michael Starzinger, 2015-02-16 20:11:27]

https://codereview.chromium.org/919703003/diff/80001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/919703003/diff/80001/src/x64/full-codegen-x64...
src/x64/full-codegen-x64.cc:2923: DoSpreadArgument(e->AsSpreadOperation(), rax);
The $rax register here will most likely no longer be set to the argument count
unless the SpreadOperation is the very first argument, because evaluation of
non-spread arguments can clobber $rax. This might explain why method calls are
"failing pretty badly".

[caitp (gmail), 2015-02-16 21:41:51]

https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2236: VisitForValue(expr->result_value());
On 2015/02/16 19:58:58, Michael Starzinger wrote:
> I am not sure how this is supposed to work in the end. But if I understand
> correctly then the operand stack size in full-codegen is not statically known
> within the loop, because each iteration over the loop pushes one element in
the
> operand stack.
> 
> In TurboFan (as well as in Crankshaft for that matter) the environment is a
> static simulation of the operand stack (similar to an abstract
interpretation).
> But here the operand stack cannot be simulated.

I don't think I ever finished the AstGraphBuilder portion, so I wouldn't pay
much attention to it, it probably doesn't make sense.

for now, maybe it makes more sense to just desugar spread calls into
Function.apply() as suggested previously.

[Michael Starzinger, 2015-02-16 22:08:44]

On 2015/02/16 21:41:51, caitp wrote:
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> src/compiler/ast-graph-builder.cc:2236: VisitForValue(expr->result_value());
> On 2015/02/16 19:58:58, Michael Starzinger wrote:
> > I am not sure how this is supposed to work in the end. But if I understand
> > correctly then the operand stack size in full-codegen is not statically
known
> > within the loop, because each iteration over the loop pushes one element in
> the
> > operand stack.
> > 
> > In TurboFan (as well as in Crankshaft for that matter) the environment is a
> > static simulation of the operand stack (similar to an abstract
> interpretation).
> > But here the operand stack cannot be simulated.
> 
> I don't think I ever finished the AstGraphBuilder portion, so I wouldn't pay
> much attention to it, it probably doesn't make sense.
> 
> for now, maybe it makes more sense to just desugar spread calls into
> Function.apply() as suggested previously.

+1 on desugaring to Function.apply()

Short-term it will be slower than using full-codegen's operand stack to store
the argument values, because they need to be stored into the heap (i.e.
allocation plus write-barrier and all). But long-term it would give us more
freedom to find one common approach to optimize both spread-calls and
Function.apply without being restricted to a specific operand stack layout in
full-codegen. Just my two cents.

Making variable-sized operand stacks work with the deoptimizer would be a real
pain I think.

[titzer, 2015-02-17 08:49:50]

On 2015/02/16 22:08:44, Michael Starzinger wrote:
> On 2015/02/16 21:41:51, caitp wrote:
> >
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> > File src/compiler/ast-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> > src/compiler/ast-graph-builder.cc:2236: VisitForValue(expr->result_value());
> > On 2015/02/16 19:58:58, Michael Starzinger wrote:
> > > I am not sure how this is supposed to work in the end. But if I understand
> > > correctly then the operand stack size in full-codegen is not statically
> known
> > > within the loop, because each iteration over the loop pushes one element
in
> > the
> > > operand stack.
> > > 
> > > In TurboFan (as well as in Crankshaft for that matter) the environment is
a
> > > static simulation of the operand stack (similar to an abstract
> > interpretation).
> > > But here the operand stack cannot be simulated.
> > 
> > I don't think I ever finished the AstGraphBuilder portion, so I wouldn't pay
> > much attention to it, it probably doesn't make sense.
> > 
> > for now, maybe it makes more sense to just desugar spread calls into
> > Function.apply() as suggested previously.
> 
> +1 on desugaring to Function.apply()
> 
> Short-term it will be slower than using full-codegen's operand stack to store
> the argument values, because they need to be stored into the heap (i.e.
> allocation plus write-barrier and all). But long-term it would give us more
> freedom to find one common approach to optimize both spread-calls and
> Function.apply without being restricted to a specific operand stack layout in
> full-codegen. Just my two cents.
> 
> Making variable-sized operand stacks work with the deoptimizer would be a real
> pain I think.

not lgtm,

We discussed this a bit this morning and we think that this could be more easily
done with a runtime call that uses an internal array for arguments, Array.splice
and Function.apply. That would be slower, but would be platform independent and
much easier to reason about.

[caitp (gmail), 2015-02-17 23:36:16]

On 2015/02/17 08:49:50, titzer wrote:
> On 2015/02/16 22:08:44, Michael Starzinger wrote:
> > On 2015/02/16 21:41:51, caitp wrote:
> > >
> >
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> > > File src/compiler/ast-graph-builder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/919703003/diff/80001/src/compiler/ast-graph-b...
> > > src/compiler/ast-graph-builder.cc:2236:
VisitForValue(expr->result_value());
> > > On 2015/02/16 19:58:58, Michael Starzinger wrote:
> > > > I am not sure how this is supposed to work in the end. But if I
understand
> > > > correctly then the operand stack size in full-codegen is not statically
> > known
> > > > within the loop, because each iteration over the loop pushes one element
> in
> > > the
> > > > operand stack.
> > > > 
> > > > In TurboFan (as well as in Crankshaft for that matter) the environment
is
> a
> > > > static simulation of the operand stack (similar to an abstract
> > > interpretation).
> > > > But here the operand stack cannot be simulated.
> > > 
> > > I don't think I ever finished the AstGraphBuilder portion, so I wouldn't
pay
> > > much attention to it, it probably doesn't make sense.
> > > 
> > > for now, maybe it makes more sense to just desugar spread calls into
> > > Function.apply() as suggested previously.
> > 
> > +1 on desugaring to Function.apply()
> > 
> > Short-term it will be slower than using full-codegen's operand stack to
store
> > the argument values, because they need to be stored into the heap (i.e.
> > allocation plus write-barrier and all). But long-term it would give us more
> > freedom to find one common approach to optimize both spread-calls and
> > Function.apply without being restricted to a specific operand stack layout
in
> > full-codegen. Just my two cents.
> > 
> > Making variable-sized operand stacks work with the deoptimizer would be a
real
> > pain I think.
> 
> not lgtm,
> 
> We discussed this a bit this morning and we think that this could be more
easily
> done with a runtime call that uses an internal array for arguments,
Array.splice
> and Function.apply. That would be slower, but would be platform independent
and
> much easier to reason about.

There's an alternative version at https://codereview.chromium.org/938443002/
which basically does that