Allow Turbofan optimization of Ignition generators

Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n

Committed: https://crrev.com/3cc7315eda083f940c6b476bcb55c0d43344eed3
Cr-Commit-Position: refs/heads/master@{#36134}

[neis, 2016-05-10 09:57:49]

Description was changed from

==========
Allow Turbofan optimization of ignition generators

BUG=
==========

to

==========
Allow Turbofan optimization of ignition generators

BUG=v8:4907
LOG=n
==========

[neis, 2016-05-10 10:00:37]

Description was changed from

==========
Allow Turbofan optimization of ignition generators

BUG=v8:4907
LOG=n
==========

to

==========
Allow Turbofan optimization of ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific bytecodes as
a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

[neis, 2016-05-10 10:00:54]

Description was changed from

==========
Allow Turbofan optimization of ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific bytecodes as
a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

to

==========
Allow Turbofan optimization of ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

[neis, 2016-05-10 10:03:21]

neis@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org, mstarzinger@chromium.org

[neis, 2016-05-10 10:03:22]

PTAL

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc#newcode739
src/compiler.cc:739: if (IsGeneratorFunction(info->shared_info()->kind()) &&
!use_turbofan) {
Does this make sense?

[rmcilroy, 2016-05-10 10:23:21]

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

[rmcilroy, 2016-05-10 10:23:21]

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc#newcode739
src/compiler.cc:739: if (IsGeneratorFunction(info->shared_info()->kind()) &&
!use_turbofan) {
On 2016/05/10 10:03:22, neis wrote:
> Does this make sense?

I think you also need to check for the FLAG_turbo_from_bytecode, since this is
the only situation where bytecode-graph-builder will be used (otherwise we go
via FCG then via the ASTGraphBuilder to build the TF graph, which won't work for
generators).

[Benedikt Meurer, 2016-05-10 10:32:08]

LGTM with comments/nits from my side.

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1375:
NewNode(javascript()->CallRuntime(Runtime::kGeneratorWriteRegister),
Nit: use StoreRegister or SetRegister instead of WriteRegister

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1397:
javascript()->CallRuntime(Runtime::kGeneratorReadRegister),
Nit: use LoadRegister instead of ReadRegister

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:155: CONVERT_ARG_HANDLE_CHECKED(Smi,
continuation, 1);
Use CONVERT_SMI_ARG_CHECKED here.

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:166: CONVERT_ARG_HANDLE_CHECKED(Smi, index, 1);
Use CONVERT_SMI_ARG_CHECKED here.

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:179: CONVERT_ARG_HANDLE_CHECKED(Smi, index, 1);
Use CONVERT_SMI_ARG_CHECKED here.

[Michael Starzinger, 2016-05-10 10:37:11]

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc#newcode739
src/compiler.cc:739: if (IsGeneratorFunction(info->shared_info()->kind()) &&
!use_turbofan) {
On 2016/05/10 10:23:21, rmcilroy wrote:
> On 2016/05/10 10:03:22, neis wrote:
> > Does this make sense?
> 
> I think you also need to check for the FLAG_turbo_from_bytecode, since this is
> the only situation where bytecode-graph-builder will be used (otherwise we go
> via FCG then via the ASTGraphBuilder to build the TF graph, which won't work
for
> generators).

Rather than keeping this bailout here, we could move it into
HCompilationJob::CreateGraphImpl instead. This will make Crankshaft bailout of
the compilation. TurboFan will either bailout in the AstGraphBuilder or succeed
when going through the BytecodeGraphBuilder.

Note that the heuristic in AstNumberingVisitor remains untouched. So the bailout
we are talking about here are essentially just last-resort bailouts for
correctness. They can be arbitrary late in the pipeline.

[Jarin, 2016-05-10 10:46:35]

Awesome! lgtm.

[Michael Starzinger, 2016-05-10 10:53:05]

Looking good. I like it.

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1377:
environment()->LookupRegister(interpreter::Register(i)));
nit: Better pull out the
"environment()->LookupRegister(interpreter::Register(i)" into a local variable.
The NewNode function can mutate the current environment and this pattern looks
dangerous. In this case evaluation order should be fine, but I'd prefer to
explicitly sequence it.

https://codereview.chromium.org/1957393004/diff/1/test/mjsunit/harmony/genera...
File test/mjsunit/harmony/generators.js (right):

https://codereview.chromium.org/1957393004/diff/1/test/mjsunit/harmony/genera...
test/mjsunit/harmony/generators.js:6: // Flags: --allow-natives-syntax --turbo
--turbo-from-bytecode
As discussed offline: Let's drop the --turbo --turbo-from-bytecode flags, as
those will be added by the harness with the "ignition_turbofan" variant. We'd
loose test coverage by adding the flags here.

[neis, 2016-05-10 11:31:05]

Description was changed from

==========
Allow Turbofan optimization of ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

to

==========
Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

[neis, 2016-05-10 11:32:06]

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler.cc#newcode739
src/compiler.cc:739: if (IsGeneratorFunction(info->shared_info()->kind()) &&
!use_turbofan) {
On 2016/05/10 10:37:11, Michael Starzinger wrote:
> On 2016/05/10 10:23:21, rmcilroy wrote:
> > On 2016/05/10 10:03:22, neis wrote:
> > > Does this make sense?
> > 
> > I think you also need to check for the FLAG_turbo_from_bytecode, since this
is
> > the only situation where bytecode-graph-builder will be used (otherwise we
go
> > via FCG then via the ASTGraphBuilder to build the TF graph, which won't work
> for
> > generators).
> 
> Rather than keeping this bailout here, we could move it into
> HCompilationJob::CreateGraphImpl instead. This will make Crankshaft bailout of
> the compilation. TurboFan will either bailout in the AstGraphBuilder or
succeed
> when going through the BytecodeGraphBuilder.
> 
> Note that the heuristic in AstNumberingVisitor remains untouched. So the
bailout
> we are talking about here are essentially just last-resort bailouts for
> correctness. They can be arbitrary late in the pipeline.

Done.

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1375:
NewNode(javascript()->CallRuntime(Runtime::kGeneratorWriteRegister),
On 2016/05/10 10:32:08, Benedikt Meurer wrote:
> Nit: use StoreRegister or SetRegister instead of WriteRegister

Done.

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1377:
environment()->LookupRegister(interpreter::Register(i)));
On 2016/05/10 10:53:05, Michael Starzinger wrote:
> nit: Better pull out the
> "environment()->LookupRegister(interpreter::Register(i)" into a local
variable.
> The NewNode function can mutate the current environment and this pattern looks
> dangerous. In this case evaluation order should be fine, but I'd prefer to
> explicitly sequence it.

Done.

https://codereview.chromium.org/1957393004/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1397:
javascript()->CallRuntime(Runtime::kGeneratorReadRegister),
On 2016/05/10 10:32:08, Benedikt Meurer wrote:
> Nit: use LoadRegister instead of ReadRegister

Done.

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:155: CONVERT_ARG_HANDLE_CHECKED(Smi,
continuation, 1);
On 2016/05/10 10:32:08, Benedikt Meurer wrote:
> Use CONVERT_SMI_ARG_CHECKED here.

Done.

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:166: CONVERT_ARG_HANDLE_CHECKED(Smi, index, 1);
On 2016/05/10 10:32:08, Benedikt Meurer wrote:
> Use CONVERT_SMI_ARG_CHECKED here.

Done.

https://codereview.chromium.org/1957393004/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:179: CONVERT_ARG_HANDLE_CHECKED(Smi, index, 1);
On 2016/05/10 10:32:08, Benedikt Meurer wrote:
> Use CONVERT_SMI_ARG_CHECKED here.

Done.

https://codereview.chromium.org/1957393004/diff/1/test/mjsunit/harmony/genera...
File test/mjsunit/harmony/generators.js (right):

https://codereview.chromium.org/1957393004/diff/1/test/mjsunit/harmony/genera...
test/mjsunit/harmony/generators.js:6: // Flags: --allow-natives-syntax --turbo
--turbo-from-bytecode
On 2016/05/10 10:53:05, Michael Starzinger wrote:
> As discussed offline: Let's drop the --turbo --turbo-from-bytecode flags, as
> those will be added by the harness with the "ignition_turbofan" variant. We'd
> loose test coverage by adding the flags here.

Done.

[neis, 2016-05-10 11:33:06]

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

[commit-bot: I haz the power, 2016-05-10 11:33:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1957393004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1957393004/20001

[Michael Starzinger, 2016-05-10 11:48:29]

LGTM.

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

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

[commit-bot: I haz the power, 2016-05-10 12:04:09]

Dry run: This issue passed the CQ dry run.

[neis, 2016-05-10 12:04:41]

The CQ bit was checked by neis@chromium.org

[neis, 2016-05-10 12:04:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1957393004/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2016-05-10 12:04:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1957393004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1957393004/20001

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

Description was changed from

==========
Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

to

==========
Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

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

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n
==========

to

==========
Allow Turbofan optimization of Ignition generators, first version.

In the bytecode graphbuilder, translate the two generator-specific
bytecodes as a couple of runtime calls for now.

BUG=v8:4907
LOG=n

Committed: https://crrev.com/3cc7315eda083f940c6b476bcb55c0d43344eed3
Cr-Commit-Position: refs/heads/master@{#36134}
==========

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

Patchset 2 (id:??) landed as
https://crrev.com/3cc7315eda083f940c6b476bcb55c0d43344eed3
Cr-Commit-Position: refs/heads/master@{#36134}