[Ignition/turbo] Add a NewWithSpread bytecode.

[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The NewWithSpread bytecode will probably end up having different arguments with future CLs - the constructor and the new.target should have their own regs. For now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
Committed: https://crrev.com/a25e768864362c80f060095eaefe79f53e735f63
Cr-Commit-Position: refs/heads/master@{#41542}

[petermarshall, 2016-12-05 10:37:27]

The CQ bit was checked by petermarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 10:37:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-12-05 10:43:53]

Description was changed from

==========
[Ignition/turbo] Add a new ConstructWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

BUG=v8:5659
==========

to

==========
[Ignition/turbo] Add a new ConstructWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs to
together.

BUG=v8:5659
==========

[petermarshall, 2016-12-05 10:49:24]

Description was changed from

==========
[Ignition/turbo] Add a new ConstructWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs to
together.

BUG=v8:5659
==========

to

==========
[Ignition/turbo] Add a new ConstructWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

[commit-bot: I haz the power, 2016-12-05 11:06:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 11:06:04]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-12-05 11:33:40]

petermarshall@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[petermarshall, 2016-12-05 11:33:41]

PTAL :)

[Benedikt Meurer, 2016-12-05 11:42:10]

Nice! First round of comments.

https://codereview.chromium.org/2541113004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2541113004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:243: BytecodeArrayBuilder&
ConstructWithSpread(RegisterList args);
Nit: you should add a descriptive comment.

https://codereview.chromium.org/2541113004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2541113004/diff/20001/src/objects.h#newcode1511
src/objects.h:1511: bool MustIterate();
Nit: This should have a more scary^Wdescriptive name like
CanUseElementsInsteadOfIteration() or something like that.

https://codereview.chromium.org/2541113004/diff/20001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2541113004/diff/20001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:433: // Call ReflectConstruct to do the actual
super constructor call.
It'd be nice to use Execution::New directly here, instead of the indirection via
Reflect.construct.

[petermarshall, 2016-12-05 13:30:46]

https://codereview.chromium.org/2541113004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2541113004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:243: BytecodeArrayBuilder&
ConstructWithSpread(RegisterList args);
On 2016/12/05 at 11:42:10, Benedikt Meurer wrote:
> Nit: you should add a descriptive comment.

Done

https://codereview.chromium.org/2541113004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2541113004/diff/20001/src/objects.h#newcode1511
src/objects.h:1511: bool MustIterate();
On 2016/12/05 at 11:42:10, Benedikt Meurer wrote:
> Nit: This should have a more scary^Wdescriptive name like
CanUseElementsInsteadOfIteration() or something like that.

Done

https://codereview.chromium.org/2541113004/diff/20001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2541113004/diff/20001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:433: // Call ReflectConstruct to do the actual
super constructor call.
On 2016/12/05 at 11:42:10, Benedikt Meurer wrote:
> It'd be nice to use Execution::New directly here, instead of the indirection
via Reflect.construct.

That is nice, I think we avoid allocating that JS array that way

[petermarshall, 2016-12-05 14:51:26]

The CQ bit was checked by petermarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 14:51:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-12-05 15:03:44]

Nice. Interpreter changes LGTM with naming nit addressed.

+1 to putting constructor and new.target in seperate registers once this is
handled natively by the interpreter, but this is good for now.

https://codereview.chromium.org/2541113004/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2541113004/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:160: OperandType::kRegCount)                        
                           \
Could you move this to be near New bytecode. Also could we call it NewWithSpread
or rename the existing one (in seperate CL) to be Construct to keep things
consistent.

[petermarshall, 2016-12-05 15:17:40]

The CQ bit was checked by petermarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 15:17:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 15:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 15:46:13]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-12-05 16:07:19]

On 2016/12/05 at 15:03:44, rmcilroy wrote:
> Nice. Interpreter changes LGTM with naming nit addressed.
> 
> +1 to putting constructor and new.target in seperate registers once this is
handled natively by the interpreter, but this is good for now.
> 
>
https://codereview.chromium.org/2541113004/diff/40001/src/interpreter/bytecod...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/2541113004/diff/40001/src/interpreter/bytecod...
> src/interpreter/bytecodes.h:160: OperandType::kRegCount)                      
                             \
> Could you move this to be near New bytecode. Also could we call it
NewWithSpread or rename the existing one (in seperate CL) to be Construct to
keep things consistent.

Done, thanks

[petermarshall, 2016-12-05 16:09:27]

>
https://codereview.chromium.org/2541113004/diff/20001/src/objects.h#newcode1511
> src/objects.h:1511: bool MustIterate();
> Nit: This should have a more scary^Wdescriptive name like
CanUseElementsInsteadOfIteration() or something like that.

I renamed it again because I realised this is actually the opposite of
MustIterate... it is now IterationHasObservableEffects() but I'm open to other
names still.

[Benedikt Meurer, 2016-12-07 07:32:40]

LGTM with comment.

https://codereview.chromium.org/2541113004/diff/80001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2541113004/diff/80001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:421: for (uint32_t i = 0; i < spread_length; i++)
{
Can you add a TODO here to eventually use ElementAccessors here if the C++
implementation sticks? I.e. if we still need a C++ fallback even once we turn
the fast-paths into a TFS.

[rmcilroy, 2016-12-07 07:40:31]

Nit - please update the CL title and description with the name change.

[petermarshall, 2016-12-07 08:03:53]

Description was changed from

==========
[Ignition/turbo] Add a new ConstructWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

to

==========
[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

[petermarshall, 2016-12-07 08:04:13]

Description was changed from

==========
[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The ConstructWithSpread bytecode will probably end up having different arguments
with future CLs - the constructor and the new.target should have their own regs.
For now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

to

==========
[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The NewWithSpread bytecode will probably end up having different arguments with
future CLs - the constructor and the new.target should have their own regs. For
now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

[petermarshall, 2016-12-07 08:04:56]

On 2016/12/07 at 07:40:31, rmcilroy wrote:
> Nit - please update the CL title and description with the name change.

Done, thanks =]

[petermarshall, 2016-12-07 08:05:02]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-12-07 08:05:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2541113004/#ps100001
(title: "Add todo")

[commit-bot: I haz the power, 2016-12-07 08:05:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 08:34:51]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481097902717740,
"parent_rev": "085fd4826c16054c531152f89fa782244293ceb7", "commit_rev":
"829c4dd4db70415598f3416ff820053c33c5ca7e"}

[commit-bot: I haz the power, 2016-12-07 08:34:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-07 08:35:15]

Description was changed from

==========
[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The NewWithSpread bytecode will probably end up having different arguments with
future CLs - the constructor and the new.target should have their own regs. For
now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659
==========

to

==========
[Ignition/turbo] Add a NewWithSpread bytecode.

This just calls into a runtime function for implementation currently.

Intermediate step in speeding up constructor calls containing a spread.

The NewWithSpread bytecode will probably end up having different arguments with
future CLs - the constructor and the new.target should have their own regs. For
now we are calling into the runtime function, so we need the regs together.

BUG=v8:5659

Committed: https://crrev.com/a25e768864362c80f060095eaefe79f53e735f63
Cr-Commit-Position: refs/heads/master@{#41542}
==========

[commit-bot: I haz the power, 2016-12-07 08:35:16]

Patchset 6 (id:??) landed as
https://crrev.com/a25e768864362c80f060095eaefe79f53e735f63
Cr-Commit-Position: refs/heads/master@{#41542}