[Interpreter] Generate valid FrameStates in the Bytecode Graph Builder.

[Interpreter] Generate valid FrameStates in the Bytecode Graph Builder.

Adds FrameState nodes to graphs built by the Bytecode Graph Builder, in
preparation for adding deopt support. Also adds a new
FrameStateType::kInterpretedFunction to allow for specialized deopt
stack translation for interpreted frames. Finally adds support for
disabling typed lowering of binary ops, since the current approach
relies on a FrameState hack which does not apply to interpreted frames

BUG=v8:4280
LOG=N

Committed: https://crrev.com/32211800d85f262a0d05ed3525c0a523a9e10492
Cr-Commit-Position: refs/heads/master@{#32964}

[rmcilroy, 2015-12-11 15:28:42]

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

[rmcilroy, 2015-12-11 15:28:42]

jarin@ / mstarzinger@: Please take a look. I haven't yet implemented any
translation support, but I though I would through this CL over to get some
comments. I'm fine with not landing this until I've added translation support /
tests (in a followup CL).

oth@ / mythria@: FYI.

[Jarin, 2015-12-13 20:26:30]

The overall direction looks good.

I have been under the impression that the goal for now is to make 'function f()
{ %DeoptimizeFunction(f); } f();' work. Or perhaps something like the
DeoptSimple test in test/cctest/compiler/test-run-deopt.cc. With that in mind,
there are bunch of changes that do not seem to be necessary. E.g., disabling the
typed pipeline for binops seems to be an overkill, and you also do not need the
frame state before/after ugliness yet.

Could you please separate the unnecessary changes to a different CL that can be
landed once we really need that?

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

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:150: update_mode ==
AccumulatorUpdateMode::kOutputIgnored
I do not understand why you differentiate between ignored output and pushed
output here. We only have the push/ignore flag to mirror full-code's AST context
magic (and we would love to be able to get rid of this). Why do you think you
need this?

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
states(this, iterator);
Could we please implement the minimal machinery needed for lazy deopt from
%DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter everywhere.
(For the lazy deopt, you actually only need the bailout point after, so you
would not even need this FrameStateBeforeAndAfter class just yet.)

https://codereview.chromium.org/1514413002/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:562: // TODO(rmcilroy): Implement interpreted
function translation.
Until this is implemented, could you have UNIMPLEMENTED or UNREACHABLE or
CHECK(false) here rather than silent happiness?

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
Again, I do not think we really need to disable binop reduction to make lazy
deoptimization from %DeoptimizeFunction(..) working.

[rmcilroy, 2015-12-16 15:40:57]

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

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:150: update_mode ==
AccumulatorUpdateMode::kOutputIgnored
On 2015/12/13 20:26:30, Jarin wrote:
> I do not understand why you differentiate between ignored output and pushed
> output here. We only have the push/ignore flag to mirror full-code's AST
context
> magic (and we would love to be able to get rid of this). Why do you think you
> need this?


We need this because some bytecodes don't modify the accumulator, while others
save the result in the accumulator. If we treated all bytecodes as updating the
accumulator, then the after FrameStates for the bytecodes which don't modify the
accumulator would be wrong - the translator would replace the top value in the
stack StateValues (which represents the accumulator in the interpreter) with the
result node due to the OutputFrameStateCombine::PokeAt(0). Therefore, on deopt,
the accumulator would be set to the wrong value (the result of the operation,
not the previous value in the accumulator).

Note: previously I was using OutputFrameStateCombine::Push() however this was
wrong since the stack StateValues here are only a single value which represents
the accumulator. This should have been OutputFrameStateCombine::PokeAt(0), which
I've got in the latest patch set.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
states(this, iterator);
On 2015/12/13 20:26:30, Jarin wrote:
> Could we please implement the minimal machinery needed for lazy deopt from
> %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter everywhere.
> (For the lazy deopt, you actually only need the bailout point after, so you
> would not even need this FrameStateBeforeAndAfter class just yet.)

As discussed offline, keeping this machinary in since we can support eager deopt
too with the followup CL.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:562: // TODO(rmcilroy): Implement interpreted
function translation.
On 2015/12/13 20:26:30, Jarin wrote:
> Until this is implemented, could you have UNIMPLEMENTED or UNREACHABLE or
> CHECK(false) here rather than silent happiness?

This would cause all the current bytecode-graph-builder test to fail since they
would all end up reaching this point (e.g., when adding the after framestate for
calls), even if they don't trigger a deopt.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
On 2015/12/13 20:26:30, Jarin wrote:
> Again, I do not think we really need to disable binop reduction to make lazy
> deoptimization from %DeoptimizeFunction(..) working.  

As discussed, this is for eager deopt. Unfortunately if we add any non-empty
frame-state to binary ops then the js-typed-lowering optimizations will crash
due to the "stack values" (i.e., accumulator) only having 1 value instead of the
multiple expected by the optimizations. Happy to chat offline about a plan for
enabling them for the interpreter.

[Jarin, 2015-12-17 09:44:05]

lgtm with some random comments.

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

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:150: update_mode ==
AccumulatorUpdateMode::kOutputIgnored
On 2015/12/16 15:40:56, rmcilroy wrote:
> On 2015/12/13 20:26:30, Jarin wrote:
> > I do not understand why you differentiate between ignored output and pushed
> > output here. We only have the push/ignore flag to mirror full-code's AST
> context
> > magic (and we would love to be able to get rid of this). Why do you think
you
> > need this?
> 
> 
> We need this because some bytecodes don't modify the accumulator, while others
> save the result in the accumulator. If we treated all bytecodes as updating
the
> accumulator, then the after FrameStates for the bytecodes which don't modify
the
> accumulator would be wrong - the translator would replace the top value in the
> stack StateValues (which represents the accumulator in the interpreter) with
the
> result node due to the OutputFrameStateCombine::PokeAt(0). Therefore, on
deopt,
> the accumulator would be set to the wrong value (the result of the operation,
> not the previous value in the accumulator).
> 
> Note: previously I was using OutputFrameStateCombine::Push() however this was
> wrong since the stack StateValues here are only a single value which
represents
> the accumulator. This should have been OutputFrameStateCombine::PokeAt(0),
which
> I've got in the latest patch set.

Acknowledged.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
states(this, iterator);
On 2015/12/16 15:40:56, rmcilroy wrote:
> On 2015/12/13 20:26:30, Jarin wrote:
> > Could we please implement the minimal machinery needed for lazy deopt from
> > %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter everywhere.
> > (For the lazy deopt, you actually only need the bailout point after, so you
> > would not even need this FrameStateBeforeAndAfter class just yet.)
> 
> As discussed offline, keeping this machinary in since we can support eager
deopt
> too with the followup CL.

Ack. Even though I am wondering whether the FrameStateBeforeAfter class does not
offer too much freedom to get things wrong. For example, I think it would be
wrong to capture the eager deopt environment enywhere else than at the beginning
of the bytecode. Next, the OutputIgnore/OutputInAccumulator distinction seems to
be in sync with BindAccumulator.

I am wondering whether there should not be some unified machinery that would
only know whether the bytecode can deopt or not and whether the result is stored
in the accumulator. Based on that it would build the right frame states. That
might be out of scope of this CL. However,  we should learn from Crankshaft and
make this more fool-proof.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
On 2015/12/16 15:40:57, rmcilroy wrote:
> On 2015/12/13 20:26:30, Jarin wrote:
> > Again, I do not think we really need to disable binop reduction to make lazy
> > deoptimization from %DeoptimizeFunction(..) working.  
> 
> As discussed, this is for eager deopt. Unfortunately if we add any non-empty
> frame-state to binary ops then the js-typed-lowering optimizations will crash
> due to the "stack values" (i.e., accumulator) only having 1 value instead of
the
> multiple expected by the optimizations. Happy to chat offline about a plan for
> enabling them for the interpreter. 

Acknowledged. 

We could make this hack work in the interpreter if we annotated all the JS
operations with the positions of the inputs in the environment, so that we knew
where to poke the converted inputs, but I suspect there would be some resistance
from Michi. 

Once we fully support type feedback and asm.js pipeline takes a different path,
this hack can hopefully go away (we would just deopt if the input is not number
and we had number feedback; if we would not have number feedback, we would just
use generic lowering).

[rmcilroy, 2015-12-17 14:43:48]

Added some checks to ensure we capture the before and after environment at the
right time. PTAL.

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

https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
states(this, iterator);
On 2015/12/17 09:44:05, Jarin wrote:
> On 2015/12/16 15:40:56, rmcilroy wrote:
> > On 2015/12/13 20:26:30, Jarin wrote:
> > > Could we please implement the minimal machinery needed for lazy deopt from
> > > %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter
everywhere.
> > > (For the lazy deopt, you actually only need the bailout point after, so
you
> > > would not even need this FrameStateBeforeAndAfter class just yet.)
> > 
> > As discussed offline, keeping this machinary in since we can support eager
> deopt
> > too with the followup CL.
> 
> Ack. Even though I am wondering whether the FrameStateBeforeAfter class does
not
> offer too much freedom to get things wrong. For example, I think it would be
> wrong to capture the eager deopt environment enywhere else than at the
beginning
> of the bytecode. Next, the OutputIgnore/OutputInAccumulator distinction seems
to
> be in sync with BindAccumulator.
> 
> I am wondering whether there should not be some unified machinery that would
> only know whether the bytecode can deopt or not and whether the result is
stored
> in the accumulator. Based on that it would build the right frame states. That
> might be out of scope of this CL. However,  we should learn from Crankshaft
and
> make this more fool-proof.

Good point on the fact that the OutputIgnore/OutputInAccumulator distinction is
in sync with BindAccumulator. I've changed things in the latest patchset to have
BindAccumulator take an optional state, and do the AddToNode there. I've also
added another environemnt()->RecordAfterState() for bytecodes which don't update
the accumulator, and have made FrameStateBeforeAndAfter private so that it is
only ever called from the environment.

As for capturing the wrong environment at the beginning - I can't think of a
good way of verifying this, other than to make sure that the
FrameStateBeforeAndAfter object is always created first in the Visit<Bytecode>
functions so that the environment is captured before anything is done to it.
I've added a DCHECK in the latest patchset to ensure that the
FrameStateBeforeAndAfter is up-to-date with the current environment when it goes
out of scope, which should catch situations where we make a mistake and don't
capture the correct after bytecode environment.

WDYT of the changes?

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
On 2015/12/17 09:44:05, Jarin wrote:
> On 2015/12/16 15:40:57, rmcilroy wrote:
> > On 2015/12/13 20:26:30, Jarin wrote:
> > > Again, I do not think we really need to disable binop reduction to make
lazy
> > > deoptimization from %DeoptimizeFunction(..) working.  
> > 
> > As discussed, this is for eager deopt. Unfortunately if we add any non-empty
> > frame-state to binary ops then the js-typed-lowering optimizations will
crash
> > due to the "stack values" (i.e., accumulator) only having 1 value instead of
> the
> > multiple expected by the optimizations. Happy to chat offline about a plan
for
> > enabling them for the interpreter. 
> 
> Acknowledged. 
> 
> We could make this hack work in the interpreter if we annotated all the JS
> operations with the positions of the inputs in the environment, so that we
knew
> where to poke the converted inputs, but I suspect there would be some
resistance
> from Michi. 

Yes, this was the general approach I was thinking about to solve this.

> Once we fully support type feedback and asm.js pipeline takes a different
path,
> this hack can hopefully go away (we would just deopt if the input is not
number
> and we had number feedback; if we would not have number feedback, we would
just
> use generic lowering). 

This would probably be even better :).

[rmcilroy, 2015-12-17 23:50:13]

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

[commit-bot: I haz the power, 2015-12-17 23:50:21]

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

[commit-bot: I haz the power, 2015-12-18 00:20:46]

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

[commit-bot: I haz the power, 2015-12-18 00:20:46]

Dry run: This issue passed the CQ dry run.

[Jarin, 2015-12-18 08:18:10]

On 2015/12/17 14:43:48, rmcilroy wrote:
> Added some checks to ensure we capture the before and after environment at the
> right time. PTAL.
> 
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
> states(this, iterator);
> On 2015/12/17 09:44:05, Jarin wrote:
> > On 2015/12/16 15:40:56, rmcilroy wrote:
> > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > Could we please implement the minimal machinery needed for lazy deopt
from
> > > > %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter
> everywhere.
> > > > (For the lazy deopt, you actually only need the bailout point after, so
> you
> > > > would not even need this FrameStateBeforeAndAfter class just yet.)
> > > 
> > > As discussed offline, keeping this machinary in since we can support eager
> > deopt
> > > too with the followup CL.
> > 
> > Ack. Even though I am wondering whether the FrameStateBeforeAfter class does
> not
> > offer too much freedom to get things wrong. For example, I think it would be
> > wrong to capture the eager deopt environment enywhere else than at the
> beginning
> > of the bytecode. Next, the OutputIgnore/OutputInAccumulator distinction
seems
> to
> > be in sync with BindAccumulator.
> > 
> > I am wondering whether there should not be some unified machinery that would
> > only know whether the bytecode can deopt or not and whether the result is
> stored
> > in the accumulator. Based on that it would build the right frame states.
That
> > might be out of scope of this CL. However,  we should learn from Crankshaft
> and
> > make this more fool-proof.
> 
> Good point on the fact that the OutputIgnore/OutputInAccumulator distinction
is
> in sync with BindAccumulator. I've changed things in the latest patchset to
have
> BindAccumulator take an optional state, and do the AddToNode there. I've also
> added another environemnt()->RecordAfterState() for bytecodes which don't
update
> the accumulator, and have made FrameStateBeforeAndAfter private so that it is
> only ever called from the environment.
> 
> As for capturing the wrong environment at the beginning - I can't think of a
> good way of verifying this, other than to make sure that the
> FrameStateBeforeAndAfter object is always created first in the Visit<Bytecode>
> functions so that the environment is captured before anything is done to it.
> I've added a DCHECK in the latest patchset to ensure that the
> FrameStateBeforeAndAfter is up-to-date with the current environment when it
goes
> out of scope, which should catch situations where we make a mistake and don't
> capture the correct after bytecode environment.
> 
> WDYT of the changes?

Yes, I think this is better. Thanks!

As for the state before, I am thinking about making this more declarative -
VisitBytecodes could take care of the deoptimization plumbing based on
per-opcode flags (1. needs eager deopt, 2. needs lazy deopt, 3. updates
accumulator). This requires a bit more thought, and should be done in a separate
CL (if at all).
 
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> File src/compiler/js-typed-lowering.h (right):
> 
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
> On 2015/12/17 09:44:05, Jarin wrote:
> > On 2015/12/16 15:40:57, rmcilroy wrote:
> > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > Again, I do not think we really need to disable binop reduction to make
> lazy
> > > > deoptimization from %DeoptimizeFunction(..) working.  
> > > 
> > > As discussed, this is for eager deopt. Unfortunately if we add any
non-empty
> > > frame-state to binary ops then the js-typed-lowering optimizations will
> crash
> > > due to the "stack values" (i.e., accumulator) only having 1 value instead
of
> > the
> > > multiple expected by the optimizations. Happy to chat offline about a plan
> for
> > > enabling them for the interpreter. 
> > 
> > Acknowledged. 
> > 
> > We could make this hack work in the interpreter if we annotated all the JS
> > operations with the positions of the inputs in the environment, so that we
> knew
> > where to poke the converted inputs, but I suspect there would be some
> resistance
> > from Michi. 
> 
> Yes, this was the general approach I was thinking about to solve this.
> 
> > Once we fully support type feedback and asm.js pipeline takes a different
> path,
> > this hack can hopefully go away (we would just deopt if the input is not
> number
> > and we had number feedback; if we would not have number feedback, we would
> just
> > use generic lowering). 
> 
> This would probably be even better :).

I talked to Benedikt about the ton-number deopt hack. He said he would like to
use it even in places that do not have type feedback; for example, calls to
Math.min and such. I guess we will have to make it work with bytecode as well;
but it would be better to wait for Michi and negotiate some consensus on how to
do this as cleanly as possible.

LGTM!

[rmcilroy, 2015-12-18 08:39:29]

On 2015/12/18 08:18:10, Jarin wrote:
> On 2015/12/17 14:43:48, rmcilroy wrote:
> > Added some checks to ensure we capture the before and after environment at
the
> > right time. PTAL.
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> > File src/compiler/bytecode-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> > src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
> > states(this, iterator);
> > On 2015/12/17 09:44:05, Jarin wrote:
> > > On 2015/12/16 15:40:56, rmcilroy wrote:
> > > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > > Could we please implement the minimal machinery needed for lazy deopt
> from
> > > > > %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter
> > everywhere.
> > > > > (For the lazy deopt, you actually only need the bailout point after,
so
> > you
> > > > > would not even need this FrameStateBeforeAndAfter class just yet.)
> > > > 
> > > > As discussed offline, keeping this machinary in since we can support
eager
> > > deopt
> > > > too with the followup CL.
> > > 
> > > Ack. Even though I am wondering whether the FrameStateBeforeAfter class
does
> > not
> > > offer too much freedom to get things wrong. For example, I think it would
be
> > > wrong to capture the eager deopt environment enywhere else than at the
> > beginning
> > > of the bytecode. Next, the OutputIgnore/OutputInAccumulator distinction
> seems
> > to
> > > be in sync with BindAccumulator.
> > > 
> > > I am wondering whether there should not be some unified machinery that
would
> > > only know whether the bytecode can deopt or not and whether the result is
> > stored
> > > in the accumulator. Based on that it would build the right frame states.
> That
> > > might be out of scope of this CL. However,  we should learn from
Crankshaft
> > and
> > > make this more fool-proof.
> > 
> > Good point on the fact that the OutputIgnore/OutputInAccumulator distinction
> is
> > in sync with BindAccumulator. I've changed things in the latest patchset to
> have
> > BindAccumulator take an optional state, and do the AddToNode there. I've
also
> > added another environemnt()->RecordAfterState() for bytecodes which don't
> update
> > the accumulator, and have made FrameStateBeforeAndAfter private so that it
is
> > only ever called from the environment.
> > 
> > As for capturing the wrong environment at the beginning - I can't think of a
> > good way of verifying this, other than to make sure that the
> > FrameStateBeforeAndAfter object is always created first in the
Visit<Bytecode>
> > functions so that the environment is captured before anything is done to it.
> > I've added a DCHECK in the latest patchset to ensure that the
> > FrameStateBeforeAndAfter is up-to-date with the current environment when it
> goes
> > out of scope, which should catch situations where we make a mistake and
don't
> > capture the correct after bytecode environment.
> > 
> > WDYT of the changes?
> 
> Yes, I think this is better. Thanks!
> 
> As for the state before, I am thinking about making this more declarative -
> VisitBytecodes could take care of the deoptimization plumbing based on
> per-opcode flags (1. needs eager deopt, 2. needs lazy deopt, 3. updates
> accumulator). This requires a bit more thought, and should be done in a
separate
> CL (if at all).

Right, makes sense. We could probably put these flags in bytecodes.h alongside
the bytecodes definition. I'll have a think for a follow-up CL.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> > File src/compiler/js-typed-lowering.h (right):
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> > src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
> > On 2015/12/17 09:44:05, Jarin wrote:
> > > On 2015/12/16 15:40:57, rmcilroy wrote:
> > > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > > Again, I do not think we really need to disable binop reduction to
make
> > lazy
> > > > > deoptimization from %DeoptimizeFunction(..) working.  
> > > > 
> > > > As discussed, this is for eager deopt. Unfortunately if we add any
> non-empty
> > > > frame-state to binary ops then the js-typed-lowering optimizations will
> > crash
> > > > due to the "stack values" (i.e., accumulator) only having 1 value
instead
> of
> > > the
> > > > multiple expected by the optimizations. Happy to chat offline about a
plan
> > for
> > > > enabling them for the interpreter. 
> > > 
> > > Acknowledged. 
> > > 
> > > We could make this hack work in the interpreter if we annotated all the JS
> > > operations with the positions of the inputs in the environment, so that we
> > knew
> > > where to poke the converted inputs, but I suspect there would be some
> > resistance
> > > from Michi. 
> > 
> > Yes, this was the general approach I was thinking about to solve this.
> > 
> > > Once we fully support type feedback and asm.js pipeline takes a different
> > path,
> > > this hack can hopefully go away (we would just deopt if the input is not
> > number
> > > and we had number feedback; if we would not have number feedback, we would
> > just
> > > use generic lowering). 
> > 
> > This would probably be even better :).
> 
> I talked to Benedikt about the ton-number deopt hack. He said he would like to
> use it even in places that do not have type feedback; for example, calls to
> Math.min and such. I guess we will have to make it work with bytecode as well;
> but it would be better to wait for Michi and negotiate some consensus on how
to
> do this as cleanly as possible.

Acknowledged

> LGTM!

Thanks for the reviews! I'll land now (if Michigan has any feedback I'll address
it in a follow-up CL.

[rmcilroy, 2015-12-18 08:39:30]

On 2015/12/18 08:18:10, Jarin wrote:
> On 2015/12/17 14:43:48, rmcilroy wrote:
> > Added some checks to ensure we capture the before and after environment at
the
> > right time. PTAL.
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> > File src/compiler/bytecode-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/bytecode-graph...
> > src/compiler/bytecode-graph-builder.cc:509: FrameStateBeforeAndAfter
> > states(this, iterator);
> > On 2015/12/17 09:44:05, Jarin wrote:
> > > On 2015/12/16 15:40:56, rmcilroy wrote:
> > > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > > Could we please implement the minimal machinery needed for lazy deopt
> from
> > > > > %DeoptimizeFunction() before sprinkling FrameStateBeforeAndAfter
> > everywhere.
> > > > > (For the lazy deopt, you actually only need the bailout point after,
so
> > you
> > > > > would not even need this FrameStateBeforeAndAfter class just yet.)
> > > > 
> > > > As discussed offline, keeping this machinary in since we can support
eager
> > > deopt
> > > > too with the followup CL.
> > > 
> > > Ack. Even though I am wondering whether the FrameStateBeforeAfter class
does
> > not
> > > offer too much freedom to get things wrong. For example, I think it would
be
> > > wrong to capture the eager deopt environment enywhere else than at the
> > beginning
> > > of the bytecode. Next, the OutputIgnore/OutputInAccumulator distinction
> seems
> > to
> > > be in sync with BindAccumulator.
> > > 
> > > I am wondering whether there should not be some unified machinery that
would
> > > only know whether the bytecode can deopt or not and whether the result is
> > stored
> > > in the accumulator. Based on that it would build the right frame states.
> That
> > > might be out of scope of this CL. However,  we should learn from
Crankshaft
> > and
> > > make this more fool-proof.
> > 
> > Good point on the fact that the OutputIgnore/OutputInAccumulator distinction
> is
> > in sync with BindAccumulator. I've changed things in the latest patchset to
> have
> > BindAccumulator take an optional state, and do the AddToNode there. I've
also
> > added another environemnt()->RecordAfterState() for bytecodes which don't
> update
> > the accumulator, and have made FrameStateBeforeAndAfter private so that it
is
> > only ever called from the environment.
> > 
> > As for capturing the wrong environment at the beginning - I can't think of a
> > good way of verifying this, other than to make sure that the
> > FrameStateBeforeAndAfter object is always created first in the
Visit<Bytecode>
> > functions so that the environment is captured before anything is done to it.
> > I've added a DCHECK in the latest patchset to ensure that the
> > FrameStateBeforeAndAfter is up-to-date with the current environment when it
> goes
> > out of scope, which should catch situations where we make a mistake and
don't
> > capture the correct after bytecode environment.
> > 
> > WDYT of the changes?
> 
> Yes, I think this is better. Thanks!
> 
> As for the state before, I am thinking about making this more declarative -
> VisitBytecodes could take care of the deoptimization plumbing based on
> per-opcode flags (1. needs eager deopt, 2. needs lazy deopt, 3. updates
> accumulator). This requires a bit more thought, and should be done in a
separate
> CL (if at all).

Right, makes sense. We could probably put these flags in bytecodes.h alongside
the bytecodes definition. I'll have a think for a follow-up CL.

https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> > File src/compiler/js-typed-lowering.h (right):
> > 
> >
>
https://codereview.chromium.org/1514413002/diff/1/src/compiler/js-typed-lower...
> > src/compiler/js-typed-lowering.h:38: kDisableBinaryOpReduction = 1u << 1,
> > On 2015/12/17 09:44:05, Jarin wrote:
> > > On 2015/12/16 15:40:57, rmcilroy wrote:
> > > > On 2015/12/13 20:26:30, Jarin wrote:
> > > > > Again, I do not think we really need to disable binop reduction to
make
> > lazy
> > > > > deoptimization from %DeoptimizeFunction(..) working.  
> > > > 
> > > > As discussed, this is for eager deopt. Unfortunately if we add any
> non-empty
> > > > frame-state to binary ops then the js-typed-lowering optimizations will
> > crash
> > > > due to the "stack values" (i.e., accumulator) only having 1 value
instead
> of
> > > the
> > > > multiple expected by the optimizations. Happy to chat offline about a
plan
> > for
> > > > enabling them for the interpreter. 
> > > 
> > > Acknowledged. 
> > > 
> > > We could make this hack work in the interpreter if we annotated all the JS
> > > operations with the positions of the inputs in the environment, so that we
> > knew
> > > where to poke the converted inputs, but I suspect there would be some
> > resistance
> > > from Michi. 
> > 
> > Yes, this was the general approach I was thinking about to solve this.
> > 
> > > Once we fully support type feedback and asm.js pipeline takes a different
> > path,
> > > this hack can hopefully go away (we would just deopt if the input is not
> > number
> > > and we had number feedback; if we would not have number feedback, we would
> > just
> > > use generic lowering). 
> > 
> > This would probably be even better :).
> 
> I talked to Benedikt about the ton-number deopt hack. He said he would like to
> use it even in places that do not have type feedback; for example, calls to
> Math.min and such. I guess we will have to make it work with bytecode as well;
> but it would be better to wait for Michi and negotiate some consensus on how
to
> do this as cleanly as possible.

Acknowledged

> LGTM!

Thanks for the reviews! I'll land now (if Michigan has any feedback I'll address
it in a follow-up CL.

[rmcilroy, 2015-12-18 08:39:42]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-12-18 08:39:53]

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

[commit-bot: I haz the power, 2015-12-18 08:41:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-18 08:41:36]

Description was changed from

==========
[Interpreter] Generate valid FrameStates in the Bytecode Graph Builder.

Adds FrameState nodes to graphs built by the Bytecode Graph Builder, in
preparation for adding deopt support. Also adds a new
FrameStateType::kInterpretedFunction to allow for specialized deopt
stack translation for interpreted frames. Finally adds support for
disabling typed lowering of binary ops, since the current approach
relies on a FrameState hack which does not apply to interpreted frames

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Generate valid FrameStates in the Bytecode Graph Builder.

Adds FrameState nodes to graphs built by the Bytecode Graph Builder, in
preparation for adding deopt support. Also adds a new
FrameStateType::kInterpretedFunction to allow for specialized deopt
stack translation for interpreted frames. Finally adds support for
disabling typed lowering of binary ops, since the current approach
relies on a FrameState hack which does not apply to interpreted frames

BUG=v8:4280
LOG=N

Committed: https://crrev.com/32211800d85f262a0d05ed3525c0a523a9e10492
Cr-Commit-Position: refs/heads/master@{#32964}
==========

[commit-bot: I haz the power, 2015-12-18 08:41:36]

Patchset 4 (id:??) landed as
https://crrev.com/32211800d85f262a0d05ed3525c0a523a9e10492
Cr-Commit-Position: refs/heads/master@{#32964}