[interpreter] Filter expression positions at source.

[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280, chromium:615979
LOG=N

Committed: https://crrev.com/769d33261937b0a38dfa365b3d48d9460f61ee2f
Cr-Commit-Position: refs/heads/master@{#36863}

[oth, 2016-06-06 05:11:16]

Description was changed from

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

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

to

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280,chromium:615979
LOG=N
==========

[oth, 2016-06-06 05:16:11]

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

[oth, 2016-06-06 05:16:12]

Ross, PTAL, thanks.

[rmcilroy, 2016-06-07 10:21:41]

Please also let Yang have a look at this before landing.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:85:
!Bytecodes::IsWithoutExternalSideEffects(node->bytecode())) {
Please pull out a CanFilterPosition function for clarity

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:87: latest_source_info_.set_invalid();
I think you should set_invalid unconditionally, otherwise you might attach it to
a later node.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:729:
latest_source_info_.set_invalid();
Why do you need to set this as invalid, won't Update  update latest_source_info_
appropriately if both are expressions (and if not, it should).

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
File src/interpreter/bytecode-pipeline.h (left):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
src/interpreter/bytecode-pipeline.h:88:
DISALLOW_COPY_AND_ASSIGN(BytecodeSourceInfo);
I don't think you need to remove this.

[oth, 2016-06-08 09:18:59]

On 2016/06/07 10:21:41, rmcilroy wrote:
> Please also let Yang have a look at this before landing.
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
> File src/interpreter/bytecode-array-builder.cc (right):
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
> src/interpreter/bytecode-array-builder.cc:85:
> !Bytecodes::IsWithoutExternalSideEffects(node->bytecode())) {
> Please pull out a CanFilterPosition function for clarity
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
> src/interpreter/bytecode-array-builder.cc:87:
latest_source_info_.set_invalid();
> I think you should set_invalid unconditionally, otherwise you might attach it
to
> a later node.
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
> src/interpreter/bytecode-array-builder.cc:729:
> latest_source_info_.set_invalid();
> Why do you need to set this as invalid, won't Update  update
latest_source_info_
> appropriately if both are expressions (and if not, it should).
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
> File src/interpreter/bytecode-pipeline.h (left):
> 
>
https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
> src/interpreter/bytecode-pipeline.h:88:
> DISALLOW_COPY_AND_ASSIGN(BytecodeSourceInfo);
> I don't think you need to remove this.

Thanks, comments incorporated. Adding Yang.

[oth, 2016-06-08 09:19:40]

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

[oth, 2016-06-08 09:19:41]

+Yang, PTAL.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:85:
!Bytecodes::IsWithoutExternalSideEffects(node->bytecode())) {
On 2016/06/07 10:21:41, rmcilroy wrote:
> Please pull out a CanFilterPosition function for clarity

Added NeedExpressionPosition(Bytecode).

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:87: latest_source_info_.set_invalid();
On 2016/06/07 10:21:41, rmcilroy wrote:
> I think you should set_invalid unconditionally, otherwise you might attach it
to
> a later node.

The goal is to defer writing the expression position until we discover it's
needed. If we invalidate it unconditionally, it can't be applied to where it's
needed later, e.g.

E > Star r3
    LdaGlobal ...

Should become:

    Star r3
E > LdaGlobal ...

Not:

    Star r3
    LdaGlobal ...

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:729:
latest_source_info_.set_invalid();
On 2016/06/07 10:21:41, rmcilroy wrote:
> Why do you need to set this as invalid, won't Update  update
latest_source_info_
> appropriately if both are expressions (and if not, it should).

Update() does not do the right thing. I don't want to tweak Update here as I'd
like to get rid of it entirely.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
File src/interpreter/bytecode-pipeline.h (left):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-pi...
src/interpreter/bytecode-pipeline.h:88:
DISALLOW_COPY_AND_ASSIGN(BytecodeSourceInfo);
On 2016/06/07 10:21:41, rmcilroy wrote:
> I don't think you need to remove this.

The code assigns to a BytecodeSourceInfo in AttachSourceInfo. This avoids the
messy/costly logic of Update().

[oth, 2016-06-08 09:22:43]

https://codereview.chromium.org/2038323002/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/BasicLoops.golden (right):

https://codereview.chromium.org/2038323002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/BasicLoops.golden:122: /*   42 E>
*/ B(StackCheck),
TODO(oth): Check whether the old expression position was wrong or we're now
doing the wrong thing here.

[oth, 2016-06-08 10:07:23]

https://codereview.chromium.org/2038323002/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/BasicLoops.golden (right):

https://codereview.chromium.org/2038323002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/BasicLoops.golden:122: /*   42 E>
*/ B(StackCheck),
On 2016/06/08 09:22:43, oth wrote:
> TODO(oth): Check whether the old expression position was wrong or we're now
> doing the wrong thing here.

Done. CL broke the statement positions on StackCheck. Fixed. Slightly longer
term I really want to clean-up BytecodeSourceInfo, but not in this CL.

[Yang, 2016-06-08 13:04:55]

LGTM. Very nice!

https://codereview.chromium.org/2038323002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2038323002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:319: static bool
NeedExpressionPosition(Bytecode bytecode);
I wonder whether that should belong in interpreter::Bytecodes. We have a bunch
of static functions like IsCallOrNew, IsBytecodeWithScalableOperands,
IsWithoutExternalSideEffects etc.

By that convention it would also be singular, i.e. NeedsExpressionPosition.

[rmcilroy, 2016-06-08 13:45:11]

LGTM, thanks

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:87: latest_source_info_.set_invalid();
On 2016/06/08 09:19:41, oth wrote:
> On 2016/06/07 10:21:41, rmcilroy wrote:
> > I think you should set_invalid unconditionally, otherwise you might attach
it
> to
> > a later node.
> 
> The goal is to defer writing the expression position until we discover it's
> needed. If we invalidate it unconditionally, it can't be applied to where it's
> needed later, e.g.
> 
> E > Star r3
>     LdaGlobal ...
> 
> Should become:
> 
>     Star r3
> E > LdaGlobal ...
> 
> Not:
> 
>     Star r3
>     LdaGlobal ...

I see, makes sense. Could you add a comment here as such.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:729:
latest_source_info_.set_invalid();
On 2016/06/08 09:19:41, oth wrote:
> On 2016/06/07 10:21:41, rmcilroy wrote:
> > Why do you need to set this as invalid, won't Update  update
> latest_source_info_
> > appropriately if both are expressions (and if not, it should).
> 
> Update() does not do the right thing. I don't want to tweak Update here as I'd
> like to get rid of it entirely.

I'm fine with getting rid of Update (I agree the logic in there is hard to make
sense of), however I'd like to ensure that we keep the logic for whether a
source position should overwrite another one be centralized in one place -
whether that means having a couple of additional functions in SourcePositionInfo
with different semantics or one which takes some additional parameters to
specify the situation.

For this CL, I'm fine with doing this, but please add a comment as to why it is
set invalid and a TODO with how you intend to deal with this when Update
disappears.

https://codereview.chromium.org/2038323002/diff/60001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2038323002/diff/60001/src/flag-definitions.h#...
src/flag-definitions.h:310: DEFINE_BOOL(ignition_filter_positions, true, "filter
source positions early")
Nit - this is only expression positions and it's unclear what "early" means in
this context.

How about ignition_eliminate_unecessary_expression_positions and a similar
comment?

https://codereview.chromium.org/2038323002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-source-positions.cc (right):

https://codereview.chromium.org/2038323002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-source-positions.cc:233: optimization_bitmap,
"return some_global[name];", "name", "'a'"));
Can this just be added to kTestScripts ?

[oth, 2016-06-08 15:04:21]

Thanks comments incorporated.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:87: latest_source_info_.set_invalid();
On 2016/06/08 13:45:11, rmcilroy wrote:
> On 2016/06/08 09:19:41, oth wrote:
> > On 2016/06/07 10:21:41, rmcilroy wrote:
> > > I think you should set_invalid unconditionally, otherwise you might attach
> it
> > to
> > > a later node.
> > 
> > The goal is to defer writing the expression position until we discover it's
> > needed. If we invalidate it unconditionally, it can't be applied to where
it's
> > needed later, e.g.
> > 
> > E > Star r3
> >     LdaGlobal ...
> > 
> > Should become:
> > 
> >     Star r3
> > E > LdaGlobal ...
> > 
> > Not:
> > 
> >     Star r3
> >     LdaGlobal ...
> 
> I see, makes sense. Could you add a comment here as such.

Done.

https://codereview.chromium.org/2038323002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:729:
latest_source_info_.set_invalid();
On 2016/06/08 13:45:11, rmcilroy wrote:
> On 2016/06/08 09:19:41, oth wrote:
> > On 2016/06/07 10:21:41, rmcilroy wrote:
> > > Why do you need to set this as invalid, won't Update  update
> > latest_source_info_
> > > appropriately if both are expressions (and if not, it should).
> > 
> > Update() does not do the right thing. I don't want to tweak Update here as
I'd
> > like to get rid of it entirely.
> 
> I'm fine with getting rid of Update (I agree the logic in there is hard to
make
> sense of), however I'd like to ensure that we keep the logic for whether a
> source position should overwrite another one be centralized in one place -
> whether that means having a couple of additional functions in
SourcePositionInfo
> with different semantics or one which takes some additional parameters to
> specify the situation.

Amen.

> For this CL, I'm fine with doing this, but please add a comment as to why it
is
> set invalid and a TODO with how you intend to deal with this when Update
> disappears.

Done.

https://codereview.chromium.org/2038323002/diff/60001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2038323002/diff/60001/src/flag-definitions.h#...
src/flag-definitions.h:310: DEFINE_BOOL(ignition_filter_positions, true, "filter
source positions early")
On 2016/06/08 13:45:11, rmcilroy wrote:
> Nit - this is only expression positions and it's unclear what "early" means in
> this context.
> 
> How about ignition_eliminate_unecessary_expression_positions and a similar
> comment?

Renamed and re-worded help text. Not quite the war-and-peace proposed, but
hopefully clearer?

https://codereview.chromium.org/2038323002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2038323002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:319: static bool
NeedExpressionPosition(Bytecode bytecode);
On 2016/06/08 13:04:54, Yang wrote:
> I wonder whether that should belong in interpreter::Bytecodes. We have a bunch
> of static functions like IsCallOrNew, IsBytecodeWithScalableOperands,
> IsWithoutExternalSideEffects etc.
> 
> By that convention it would also be singular, i.e. NeedsExpressionPosition.

Yes, it could do. The only quirk with this is that it depends on a v8 flag and
none of the other Bytecodes methods do so it's behaviour is not independent of
options.

I've tweaked to put it in a private namespace so it's no longer exposed. Happy
to put it in Bytecodes or leave as-is if anyone has a stronger opinion on this.

https://codereview.chromium.org/2038323002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-source-positions.cc (right):

https://codereview.chromium.org/2038323002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-source-positions.cc:233: optimization_bitmap,
"return some_global[name];", "name", "'a'"));
On 2016/06/08 13:45:11, rmcilroy wrote:
> Can this just be added to kTestScripts ?

Done.

[oth, 2016-06-09 13:02:49]

The CQ bit was checked by oth@chromium.org

[oth, 2016-06-09 13:02:50]

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

[commit-bot: I haz the power, 2016-06-09 13:02:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038323002/100001

[commit-bot: I haz the power, 2016-06-09 13:32:40]

Description was changed from

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280,chromium:615979
LOG=N
==========

to

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280,chromium:615979
LOG=N
==========

[commit-bot: I haz the power, 2016-06-09 13:32:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-09 13:33:39]

Description was changed from

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280,chromium:615979
LOG=N
==========

to

==========
[interpreter] Filter expression positions at source.

With this change the bytecode array builder only emits expression
positions for bytecodes that can throw. This allows more peephole
optimization opportunities and results in smaller code.

BUG=v8:4280,chromium:615979
LOG=N

Committed: https://crrev.com/769d33261937b0a38dfa365b3d48d9460f61ee2f
Cr-Commit-Position: refs/heads/master@{#36863}
==========

[commit-bot: I haz the power, 2016-06-09 13:33:41]

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