[Interpreter] Preserve source positions in peephole optimizer.

[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/e43fbde72b38160c4f9860ed4208570b1e1e6ef9
Cr-Commit-Position: refs/heads/master@{#36439}

[oth, 2016-05-20 10:19:33]

oth@chromium.org changed reviewers:
+ rmcilroy@chromium.org, yangguo@chromium.org

[oth, 2016-05-20 10:19:33]

Yang, Ross, PTAL, this fixes the expression elision problem that was occurring
in the output from bytecode register optimization combined with the new peephole
optimizer.

There is a table based alternative to the peephole optimizer in this CL and it'd
be easy to get back some of the space lost here, e.g. specialize the NOP+Star
case that stands out in IfConditions.golden.

Building layout tests now.

Thanks
Orion

[rmcilroy, 2016-05-20 11:08:03]

> There is a table based alternative to the peephole optimizer in this CL and
it'd
> be easy to get back some of the space lost here, e.g. specialize the NOP+Star
> case that stands out in IfConditions.golden.

I'm not sure what you mean by this - I'm guessing you mean the table based
alternative is in a different CL, not this one? 

Also, how could we specialize NOP+Star in the IfConditions.golden? If we are
intending to keep all source positions after this change then I'm not sure how
we could do that since the NOPs and Stars have different source positions. 

Maybe we should only preserve all statement positions and allow eliding
expression positions?

https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:170: current->source_info() !=
last_.source_info()) {
I think this would be clearer as:

// The last bytecode can't be elided without dropping a source
// position.
if (!current->source_info().is_valid() || current->source_info() ==
last_.source_info()) {
  // Transfer source position from bytecode that's being dropped
  // to its successor.
  current->source_info().Update(last_.source_info());
  InvalidateLast();
}

[oth, 2016-05-20 12:31:01]

On 2016/05/20 11:08:03, rmcilroy wrote:
> > There is a table based alternative to the peephole optimizer in this CL and
> it'd
> > be easy to get back some of the space lost here, e.g. specialize the
NOP+Star
> > case that stands out in IfConditions.golden.
> 
> I'm not sure what you mean by this - I'm guessing you mean the table based
> alternative is in a different CL, not this one? 
> 
> Also, how could we specialize NOP+Star in the IfConditions.golden? If we are
> intending to keep all source positions after this change then I'm not sure how
> we could do that since the NOPs and Stars have different source positions. 

In the table there is a specific rule that understands this very artificial case
which
only needs the source positions preserving.

> Maybe we should only preserve all statement positions and allow eliding
> expression positions?

Do you have specific examples in mind that benefit from this?

We need to be really careful eliding expression positions to preserve stack
trace information. The
expression information often some distance from the bytecode that could throw.
We very rarely get
back-to-back bytecodes with expression positions associated.

>
https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
> File src/interpreter/bytecode-peephole-optimizer.cc (right):
> 
>
https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
> src/interpreter/bytecode-peephole-optimizer.cc:170: current->source_info() !=
> last_.source_info()) {
> I think this would be clearer as:
> 
> // The last bytecode can't be elided without dropping a source
> // position.
> if (!current->source_info().is_valid() || current->source_info() ==
> last_.source_info()) {
>   // Transfer source position from bytecode that's being dropped
>   // to its successor.
>   current->source_info().Update(last_.source_info());
>   InvalidateLast();
> }

Let's resolve your previous point first.

[rmcilroy, 2016-05-20 12:55:52]

On 2016/05/20 12:31:01, oth wrote:
> On 2016/05/20 11:08:03, rmcilroy wrote:
> > > There is a table based alternative to the peephole optimizer in this CL
and
> > it'd
> > > be easy to get back some of the space lost here, e.g. specialize the
> NOP+Star
> > > case that stands out in IfConditions.golden.
> > 
> > I'm not sure what you mean by this - I'm guessing you mean the table based
> > alternative is in a different CL, not this one? 
> > 
> > Also, how could we specialize NOP+Star in the IfConditions.golden? If we are
> > intending to keep all source positions after this change then I'm not sure
how
> > we could do that since the NOPs and Stars have different source positions. 
> 
> In the table there is a specific rule that understands this very artificial
case
> which
> only needs the source positions preserving.
> 
> > Maybe we should only preserve all statement positions and allow eliding
> > expression positions?
> 
> Do you have specific examples in mind that benefit from this?
> 
> We need to be really careful eliding expression positions to preserve stack
> trace information. The
> expression information often some distance from the bytecode that could throw.
> We very rarely get
> back-to-back bytecodes with expression positions associated.
>

As discussed offline, my point was more that I don't think we should have a
special rule for NOP+Star which treats source positions differently from other
rules - the rule should either be to globally always emit expression positions,
or allow all peephole optimizations to potentially drop them. I agree, this is
almost certainly not an issue in the real world, and we should land this CL and
see if it causes any impact on the perf bots.

[oth, 2016-05-20 14:55:53]

On 2016/05/20 12:55:52, rmcilroy wrote:
> On 2016/05/20 12:31:01, oth wrote:
> > On 2016/05/20 11:08:03, rmcilroy wrote:
> > > > There is a table based alternative to the peephole optimizer in this CL
> and
> > > it'd
> > > > be easy to get back some of the space lost here, e.g. specialize the
> > NOP+Star
> > > > case that stands out in IfConditions.golden.
> > > 
> > > I'm not sure what you mean by this - I'm guessing you mean the table based
> > > alternative is in a different CL, not this one? 
> > > 
> > > Also, how could we specialize NOP+Star in the IfConditions.golden? If we
are
> > > intending to keep all source positions after this change then I'm not sure
> how
> > > we could do that since the NOPs and Stars have different source positions.

> > 
> > In the table there is a specific rule that understands this very artificial
> case
> > which
> > only needs the source positions preserving.
> > 
> > > Maybe we should only preserve all statement positions and allow eliding
> > > expression positions?
> > 
> > Do you have specific examples in mind that benefit from this?
> > 
> > We need to be really careful eliding expression positions to preserve stack
> > trace information. The
> > expression information often some distance from the bytecode that could
throw.
> > We very rarely get
> > back-to-back bytecodes with expression positions associated.
> >
> 
> As discussed offline, my point was more that I don't think we should have a
> special rule for NOP+Star which treats source positions differently from other
> rules - the rule should either be to globally always emit expression
positions,
> or allow all peephole optimizations to potentially drop them. I agree, this is
> almost certainly not an issue in the real world, and we should land this CL
and
> see if it causes any impact on the perf bots.

I don't have any data that favours the suggestion about eliding expression
positions. It almost certainly requires additional matching rules in the current
implementation and even then it'll likely have a perf impact on the current
implementation. That it can be done with the table based scheme is a statement
of fact, not necessity.

[oth, 2016-05-20 14:56:43]

https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:170: current->source_info() !=
last_.source_info()) {
On 2016/05/20 11:08:03, rmcilroy wrote:
> I think this would be clearer as:
> 
> // The last bytecode can't be elided without dropping a source
> // position.
> if (!current->source_info().is_valid() || current->source_info() ==
> last_.source_info()) {
>   // Transfer source position from bytecode that's being dropped
>   // to its successor.
>   current->source_info().Update(last_.source_info());
>   InvalidateLast();
> }

Revised with a simpler path that produces the same results. The suggestion is
subtly different from what it'd replace.

[rmcilroy, 2016-05-20 16:31:13]

LGTM

https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:170: (last_.source_info() ==
current->source_info()))) {
nit - could we just move the source position checking into CanElideLast?

[oth, 2016-05-21 07:39:21]

Patchset #3 (id:40001) has been deleted

[oth, 2016-05-21 12:32:52]

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

[commit-bot: I haz the power, 2016-05-21 12:33:02]

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

[oth, 2016-05-21 12:40:10]

Addressed nit.

Added extra logic to remove unnecessary expression positions, e..g those that
are on bytecodes with no external effects and immediately followed by a bytecode
with a statement position. This appears to be completely perf neutral locally.
The pipeline could do better (specifically for the S>Ldar (S), E>Star case by
having an extra stage to perform this simplification before the peephole
optimizer, but this seems overkill given the goal was to unblock other CLs and
move on to more pressing activities.

Still to get this working on layout tests, but at least after blowing away the
old client it runs. No amount of syncing, cleaning, changing build config, would
fix the old one.

https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:170: (last_.source_info() ==
current->source_info()))) {
On 2016/05/20 16:31:13, rmcilroy wrote:
> nit - could we just move the source position checking into CanElideLast?

Done.

[oth, 2016-05-21 12:40:37]

Description was changed from

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

The change results in slightly larger code, but should improve the
debugging experience. There's some hope of reclaiming some of the space
savings lost in the future.

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

to

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

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

[commit-bot: I haz the power, 2016-05-21 13:05:51]

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

[commit-bot: I haz the power, 2016-05-21 13:05:52]

Dry run: This issue passed the CQ dry run.

[oth, 2016-05-21 14:04:17]

On 2016/05/21 12:40:10, oth wrote:
> Addressed nit.
> 
> Added extra logic to remove unnecessary expression positions, e..g those that
> are on bytecodes with no external effects and immediately followed by a
bytecode
> with a statement position. This appears to be completely perf neutral locally.
> The pipeline could do better (specifically for the S>Ldar (S), E>Star case by
> having an extra stage to perform this simplification before the peephole
> optimizer, but this seems overkill given the goal was to unblock other CLs and
> move on to more pressing activities.
> 
> Still to get this working on layout tests, but at least after blowing away the
> old client it runs. No amount of syncing, cleaning, changing build config,
would
> fix the old one.
> 
>
https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecode-peephole-optimizer.cc (right):
> 
>
https://codereview.chromium.org/1998203002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-peephole-optimizer.cc:170: (last_.source_info() ==
> current->source_info()))) {
> On 2016/05/20 16:31:13, rmcilroy wrote:
> > nit - could we just move the source position checking into CanElideLast?
> 
> Done.

The same 13 layout tests fail with and without this CL.

[Yang, 2016-05-23 07:54:20]

https://codereview.chromium.org/1998203002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:162: //     |  Expr  |  YES   |  
NO   |  MAYBE |
Wouldn't this line be YES | MAYBE | MAYBE?

If we cannot throw between two expression positions, we only need the second
expression position, right?

[oth, 2016-05-23 09:05:46]

Thanks, comment incorporated.

https://codereview.chromium.org/1998203002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-peephole-optimizer.cc:162: //     |  Expr  |  YES   |  
NO   |  MAYBE |
On 2016/05/23 07:54:20, Yang wrote:
> Wouldn't this line be YES | MAYBE | MAYBE?
> 
> If we cannot throw between two expression positions, we only need the second
> expression position, right?

Yes. Updated.

[rmcilroy, 2016-05-23 10:59:26]

LGTM

https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:176: // this is not performed
here to keep the check simple.
nit - /s/but this is not.../which is checked in UpdateLastAndCurrentBytecodes to
keep the check here simple.

[oth, 2016-05-23 11:22:42]

Thanks, done.

https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:176: // this is not performed
here to keep the check simple.
On 2016/05/23 10:59:26, rmcilroy wrote:
> nit - /s/but this is not.../which is checked in UpdateLastAndCurrentBytecodes
to
> keep the check here simple.

Done.

[oth, 2016-05-23 12:16:49]

On 2016/05/23 11:22:42, oth wrote:
> Thanks, done.
> 
>
https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecode-peephole-optimizer.cc (right):
> 
>
https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecode-peephole-optimizer.cc:176: // this is not performed
> here to keep the check simple.
> On 2016/05/23 10:59:26, rmcilroy wrote:
> > nit - /s/but this is not.../which is checked in
UpdateLastAndCurrentBytecodes
> to
> > keep the check here simple.
> 
> Done.

Yang, if you're okay with the CL, I'll land it. Otherwise, happy to address any
issues.

Thanks

[Yang, 2016-05-23 12:38:52]

On 2016/05/23 12:16:49, oth wrote:
> On 2016/05/23 11:22:42, oth wrote:
> > Thanks, done.
> > 
> >
>
https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
> > File src/interpreter/bytecode-peephole-optimizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1998203002/diff/100001/src/interpreter/byteco...
> > src/interpreter/bytecode-peephole-optimizer.cc:176: // this is not performed
> > here to keep the check simple.
> > On 2016/05/23 10:59:26, rmcilroy wrote:
> > > nit - /s/but this is not.../which is checked in
> UpdateLastAndCurrentBytecodes
> > to
> > > keep the check here simple.
> > 
> > Done.
> 
> Yang, if you're okay with the CL, I'll land it. Otherwise, happy to address
any
> issues.
> 
> Thanks

LGTM.

[oth, 2016-05-23 13:00:14]

The CQ bit was checked by oth@chromium.org

[oth, 2016-05-23 13:00:14]

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/1998203002/#ps140001
(title: "Nitlet on last patch set.")

[commit-bot: I haz the power, 2016-05-23 13:00:24]

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

[commit-bot: I haz the power, 2016-05-23 13:31:58]

Description was changed from

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

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

to

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

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

[commit-bot: I haz the power, 2016-05-23 13:31:59]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-05-23 13:33:32]

Description was changed from

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

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

to

==========
[Interpreter] Preserve source positions in peephole optimizer.

The original peephole optimizer logic in the BytecodeArrayBuilder did
not respect source positions as it was written before there were
bytecode source positions. This led to some minor differences to
FCG and was problematic when combined with pending bytecode
optimizations. This change makes the new peephole optimizer fully
respect source positions.

BUG=v8:4280
LOG=N

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

[commit-bot: I haz the power, 2016-05-23 13:33:33]

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