[interpreter] Add explicit OSR polling bytecode.

[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
Committed: https://crrev.com/a55beb68e0ededb3773affa294a71edc50621458
Cr-Commit-Position: refs/heads/master@{#38043}

[Michael Starzinger, 2016-07-25 11:08:26]

Description was changed from

==========
[interpreter] OSR STEP 2

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

to

==========
[interpreter] OSR STEP 2

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

[Michael Starzinger, 2016-07-25 11:08:26]

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

[Michael Starzinger, 2016-07-25 11:08:55]

Description was changed from

==========
[interpreter] OSR STEP 2

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

to

==========
[interpreter] OSR STEP 2

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

[Michael Starzinger, 2016-07-25 11:10:37]

Description was changed from

==========
[interpreter] OSR STEP 2

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

to

==========
[interpreter] Add explicit OSR polling bytecode.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

[Michael Starzinger, 2016-07-25 11:22:53]

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

[commit-bot: I haz the power, 2016-07-25 11:23:00]

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-25 11:51:46]

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

[commit-bot: I haz the power, 2016-07-25 11:51:47]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-07-25 12:09:39]

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

[commit-bot: I haz the power, 2016-07-25 12:09:45]

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 12:16:17]

Description was changed from

==========
[interpreter] Add explicit OSR polling bytecode.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

to

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

[commit-bot: I haz the power, 2016-07-25 12:33:33]

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

[commit-bot: I haz the power, 2016-07-25 12:33:34]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-25 12:40:24]

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:669: // on-stack replacement when armed
for the given loop nesting depth.
Should this be before the generator resume logic (i.e., immediately after
loop_builder->LoopHeader())? Not sure how OSR interacts with generators?

...

Just saw your TODO - I guess you aren't sure yet either :).

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

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:264: V(OsrPoll, AccumulatorUse::kNone,
OperandType::kFlag8)                       \
This is possibly more of an Immediate rather than Flag8. I realize you don't
need the scaling support, but I'm not sure it would cost anything to make this
OperandType::kImm for clarity, WDYT?

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1806: Node* condition = __
Int32GreaterThanOrEqual(loop_depth, osr_level);
It looks like OSR nesting level initialized to 0 in the bytecode array, and
OSRPoll always passes level zero currently. Doesn't that mean we always end up
trying to OSR?

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1817: // TODO(4764): We don't have a CallStub
method with zero arguments, add one!
Could you do that with this CL please?

[Michael Starzinger, 2016-07-25 13:40:48]

Addressed comments. PTAL.

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:669: // on-stack replacement when armed
for the given loop nesting depth.
On 2016/07/25 12:40:23, rmcilroy wrote:
> Should this be before the generator resume logic (i.e., immediately after
> loop_builder->LoopHeader())? Not sure how OSR interacts with generators?
> 
> ...
> 
> Just saw your TODO - I guess you aren't sure yet either :).

Done. Yes, I am not 100% sure about interaction with generators either yet. But
intuitively OSR entry should be done before the generator dispatch. So I moved
it up for now, but left the TODO in place.

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

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:264: V(OsrPoll, AccumulatorUse::kNone,
OperandType::kFlag8)                       \
On 2016/07/25 12:40:23, rmcilroy wrote:
> This is possibly more of an Immediate rather than Flag8. I realize you don't
> need the scaling support, but I'm not sure it would cost anything to make this
> OperandType::kImm for clarity, WDYT?

Done. No strong opinion either way, making it OperandType::kImm is fine with me.

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1806: Node* condition = __
Int32GreaterThanOrEqual(loop_depth, osr_level);
On 2016/07/25 12:40:24, rmcilroy wrote:
> It looks like OSR nesting level initialized to 0 in the bytecode array, and
> OSRPoll always passes level zero currently. Doesn't that mean we always end up
> trying to OSR?

The {OsrPoll} handler will fall-through (i.e. the 'ok' label) when {loop_depth
>= bytecode->ost_nesting_level()} and only trigger (i.e. the 'osr_armed' label)
when this condition does not hold. Hence the initialization to 0 should not
cause it to trigger.

Also, I plan to make the loop depth tracking zero based, so top-level loops
within a function will contain a OsrPoll(0) as well.

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1817: // TODO(4764): We don't have a CallStub
method with zero arguments, add one!
On 2016/07/25 12:40:24, rmcilroy wrote:
> Could you do that with this CL please?

Done.

[rmcilroy, 2016-07-26 09:26:45]

LGTM, thanks!

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:669: // on-stack replacement when armed
for the given loop nesting depth.
On 2016/07/25 13:40:48, Michael Starzinger wrote:
> On 2016/07/25 12:40:23, rmcilroy wrote:
> > Should this be before the generator resume logic (i.e., immediately after
> > loop_builder->LoopHeader())? Not sure how OSR interacts with generators?
> > 
> > ...
> > 
> > Just saw your TODO - I guess you aren't sure yet either :).
> 
> Done. Yes, I am not 100% sure about interaction with generators either yet.
But
> intuitively OSR entry should be done before the generator dispatch. So I moved
> it up for now, but left the TODO in place.

Acknowledged.

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2172233002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1806: Node* condition = __
Int32GreaterThanOrEqual(loop_depth, osr_level);
On 2016/07/25 13:40:48, Michael Starzinger wrote:
> On 2016/07/25 12:40:24, rmcilroy wrote:
> > It looks like OSR nesting level initialized to 0 in the bytecode array, and
> > OSRPoll always passes level zero currently. Doesn't that mean we always end
up
> > trying to OSR?
> 
> The {OsrPoll} handler will fall-through (i.e. the 'ok' label) when {loop_depth
> >= bytecode->ost_nesting_level()} and only trigger (i.e. the 'osr_armed'
label)
> when this condition does not hold. Hence the initialization to 0 should not
> cause it to trigger.

You are right, sorry I got the labels on the branch mixed up.

> Also, I plan to make the loop depth tracking zero based, so top-level loops
> within a function will contain a OsrPoll(0) as well.

Right, sounds fine now I got the condition the right way around.

[Michael Starzinger, 2016-07-26 09:34:50]

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

[commit-bot: I haz the power, 2016-07-26 09:34:54]

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 10:09:14]

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

[commit-bot: I haz the power, 2016-07-26 10:09:15]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-07-26 10:31:17]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-07-26 10:31:17]

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

[commit-bot: I haz the power, 2016-07-26 10:31:29]

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 10:34:03]

Description was changed from

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

to

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

[commit-bot: I haz the power, 2016-07-26 10:34:04]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764
==========

to

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764

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

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

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

[Michael Achenbach, 2016-07-26 11:04:45]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2184553003/ by machenbach@chromium.org.

The reason for reverting is: Bunch of breakages. Maybe bad interaction with
https://chromium.googlesource.com/v8/v8/+/e520e5da5550f0d1a975e87d6e66a2edecb...
?

E.g.:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64/builds/11607.

[Michael Starzinger, 2016-07-26 12:49:37]

Description was changed from

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764

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

to

==========
[interpreter] Add explicit OSR polling bytecode.

This adds an explicit {OsrPoll} bytecode into every loop header which
triggers on-stack replacement when armed. Note that each such bytecode
stores the static loop depths as an operand, and hence can be armed for
specific loop depths.

This also adds builtin code that triggers OSR compilation and switches
execution over to optimized code in case compilation succeeds. In case
compilation fails, the bytecode dispatch just continues unhindered.

R=rmcilroy@chromium.org
TEST=mjsunit/ignition/osr-from-bytecode
BUG=v8:4764

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