[interpreter] Implement OSR graph construction from bytecode.

[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764
Committed: https://crrev.com/1314406c4788b7d669324b90a1b2b9d123d56546
Cr-Commit-Position: refs/heads/master@{#38083}

[Michael Starzinger, 2016-07-25 15:50:42]

Description was changed from

==========
[interpreter] OSR STEP 3

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

to

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter register are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=
BUG=v8:4764
==========

[Michael Starzinger, 2016-07-25 15:51:57]

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

[commit-bot: I haz the power, 2016-07-25 15:52:01]

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

[Michael Starzinger, 2016-07-25 15:53:01]

Description was changed from

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter register are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=
BUG=v8:4764
==========

to

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764
==========

[Michael Starzinger, 2016-07-25 15:53:01]

mstarzinger@chromium.org changed reviewers:
+ jarin@chromium.org, rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-07-25 15:53:13]

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

[commit-bot: I haz the power, 2016-07-25 15:53:13]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9647)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9628)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9592)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5614)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20160)

[Michael Starzinger, 2016-07-25 16:02:28]

Jaro: PTAL.
Ross: FYI (and PTAL at compiler.cc).

[Michael Starzinger, 2016-07-26 10:51:36]

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

[commit-bot: I haz the power, 2016-07-26 10:51:46]

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-07-26 11:07:27]

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

[commit-bot: I haz the power, 2016-07-26 11:07:28]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5662)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[rmcilroy, 2016-07-26 11:23:37]

lgtm. Couple of comments for compiler.cc

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode753
src/compiler.cc:753: // from bytecode offset and overlap with actual BailoutId.
No lookup!
This comment should maybe be in the if below?

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode754
src/compiler.cc:754: bool ignition_osr = !osr_ast_id.IsNone() &&
osr_frame->is_interpreted();
Should this check just be osr_frame->is_interpreted() with a:
DCHECK_IMPLIES(ignition_osr, !osr_ast_id.IsNone()) ?

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode756
src/compiler.cc:756: if (ignition_osr && !FLAG_turbo_from_bytecode) return
MaybeHandle<Code>();
nit - newline above to make this bailout clear.

[Michael Starzinger, 2016-07-26 14:43:15]

Thanks! Comments addressed.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode753
src/compiler.cc:753: // from bytecode offset and overlap with actual BailoutId.
No lookup!
On 2016/07/26 11:23:37, rmcilroy wrote:
> This comment should maybe be in the if below?

Done.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode754
src/compiler.cc:754: bool ignition_osr = !osr_ast_id.IsNone() &&
osr_frame->is_interpreted();
On 2016/07/26 11:23:37, rmcilroy wrote:
> Should this check just be osr_frame->is_interpreted() with a:
> DCHECK_IMPLIES(ignition_osr, !osr_ast_id.IsNone()) ?

Done. As discussed offline, we still need to check that {osr_frame != nullptr}
to filter out the non-OSR case.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler.cc#newcode756
src/compiler.cc:756: if (ignition_osr && !FLAG_turbo_from_bytecode) return
MaybeHandle<Code>();
On 2016/07/26 11:23:37, rmcilroy wrote:
> nit - newline above to make this bailout clear.

Done. Also added a comment explaining this bailout.

[Jarin, 2016-07-26 14:46:36]

lgtm.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:368: Node* entry =
graph()->NewNode(common()->OsrLoopEntry(), start, start);
Could we possibly DCHECK that the control dependency has one input at this point
to make sure that the shape is what turbofan osr deconstruction expects?
(Possibly also that the existing input is OsrNormalEntry.)

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1806: AllowHandleDereference allow_deref;
Out of curiosity, why is this needed?

[Michael Starzinger, 2016-07-26 14:58:51]

Thanks! Comments addressed.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:368: Node* entry =
graph()->NewNode(common()->OsrLoopEntry(), start, start);
On 2016/07/26 14:46:36, Jarin wrote:
> Could we possibly DCHECK that the control dependency has one input at this
point
> to make sure that the shape is what turbofan osr deconstruction expects?
> (Possibly also that the existing input is OsrNormalEntry.)

Done. The first invariant yes, I added a check of the input count. The second
suggestion however will most likely not hold. The "normal entry" is right after
start. There will be other control between the start and the loop. All we can
guarantee is that the "normal entry" should control-dominate the first loop
entry, but that is hard to verify in a check.

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2171083004/diff/40001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1806: AllowHandleDereference allow_deref;
On 2016/07/26 14:46:36, Jarin wrote:
> Out of curiosity, why is this needed?

The constructor of {OsrHelper} dereferences the {SharedFunctionInfo} and the
{BytecodeArray} to compute frame size and parameter count. For the non-Ignition
case we were relying on scope analysis information, which is not available for
the {BytecodeGraphBuilder}. Note that OSR code is never compiled concurrently.

[Jarin, 2016-07-26 15:39:18]

On 2016/07/26 14:58:51, Michael Starzinger wrote:

Thanks for the explanations. I am happy now, lgtm.

[Jarin, 2016-07-26 15:40:37]

Uhm, my deleting skills are not great today. Time for vacation!

[Michael Starzinger, 2016-07-26 15:53:15]

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

[commit-bot: I haz the power, 2016-07-26 15:53:24]

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-07-26 16:09:42]

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

[commit-bot: I haz the power, 2016-07-26 16:09:43]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9718)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5705)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Michael Starzinger, 2016-07-26 17:29:51]

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

[commit-bot: I haz the power, 2016-07-26 17:30:02]

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-07-26 17:37:01]

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

[commit-bot: I haz the power, 2016-07-26 17:37:02]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/11232)

[Michael Starzinger, 2016-07-26 18:12:35]

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

[commit-bot: I haz the power, 2016-07-26 18:12:44]

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-07-26 18:44:59]

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

[commit-bot: I haz the power, 2016-07-26 18:45:00]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-07-27 08:18:19]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-07-27 08:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2171083004/#ps160001
(title: "Fix build on Windows.")

[commit-bot: I haz the power, 2016-07-27 08:18:27]

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-07-27 08:19:48]

Description was changed from

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764
==========

to

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764
==========

[commit-bot: I haz the power, 2016-07-27 08:19:50]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-27 08:21:13]

Description was changed from

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764
==========

to

==========
[interpreter] Implement OSR graph construction from bytecode.

This implements graph construction for entry via on-stack replacement
within the {BytecodeGraphBuilder}. Entry points are at loop headers
similar to previous OSR implementations. All interpreter registers are
addressable via {OsrValue} nodes in the graph. Currently we rely on
{OsrPoll} bytecodes to be placed right after loop headers (i.e. at the
targets of back edges).

R=jarin@chromium.org
BUG=v8:4764

Committed: https://crrev.com/1314406c4788b7d669324b90a1b2b9d123d56546
Cr-Commit-Position: refs/heads/master@{#38083}
==========

[commit-bot: I haz the power, 2016-07-27 08:21:14]

Patchset 9 (id:??) landed as
https://crrev.com/1314406c4788b7d669324b90a1b2b9d123d56546
Cr-Commit-Position: refs/heads/master@{#38083}