[interpreter] source positions should not be emitted for dead code.

[interpreter] source positions should not be emitted for dead code.

R=mstarzinger@chromium.org

Committed: https://crrev.com/85eff14c373e529d268945f9725af5b4350074e9
Cr-Commit-Position: refs/heads/master@{#33775}

[Michael Starzinger, 2016-02-04 14:05:18]

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

[Michael Starzinger, 2016-02-04 14:05:19]

+Ross

[rmcilroy, 2016-02-04 15:06:27]

Hmm, this makes me wonder whether we should set the source position at the same
point as outputting a bytecode (e.g., pass the statement position to
builder()->Call(...), which would have the added advantage of making sure we
always associate the source position with the correct bytecode (just now someone
could accidentally insert a new bytecode output between the source position
setting and the Call bytecode emission).

What set of bytecodes would we expect to need to set source positions on? Is it
a limited subset (like the current Call and CallNew), or could it be any
bytecode?

WDYT?

[rmcilroy, 2016-02-04 15:06:40]

https://codereview.chromium.org/1668863002/diff/1/test/mjsunit/ignition/dead-...
File test/mjsunit/ignition/dead-code-source-position.js (right):

https://codereview.chromium.org/1668863002/diff/1/test/mjsunit/ignition/dead-...
test/mjsunit/ignition/dead-code-source-position.js:5: // Flags:
--ignition-filter=f
nit - no need for --ignition-filter=f

[Yang, 2016-02-04 15:13:06]

On 2016/02/04 15:06:27, rmcilroy wrote:
> Hmm, this makes me wonder whether we should set the source position at the
same
> point as outputting a bytecode (e.g., pass the statement position to
> builder()->Call(...), which would have the added advantage of making sure we
> always associate the source position with the correct bytecode (just now
someone
> could accidentally insert a new bytecode output between the source position
> setting and the Call bytecode emission).
> 
> What set of bytecodes would we expect to need to set source positions on? Is
it
> a limited subset (like the current Call and CallNew), or could it be any
> bytecode?
> 
> WDYT?

Eventually we would have sourcr position annotations at every expression and
every statement, similar to FCG. You may be able to rule out some, but I think
generally it could be any bytecode instruction. The current way to do it is
least instrusive.

If we happen to insert a new instruction between the annotated code offset and
the actual call instruction, I would argue that annotating that new instruction
is actually the correct thing to do. Source position calculation is not exact,
but looks for the closest annotation anyways.

[Yang, 2016-02-04 15:16:32]

On 2016/02/04 15:13:06, Yang wrote:
> On 2016/02/04 15:06:27, rmcilroy wrote:
> > Hmm, this makes me wonder whether we should set the source position at the
> same
> > point as outputting a bytecode (e.g., pass the statement position to
> > builder()->Call(...), which would have the added advantage of making sure we
> > always associate the source position with the correct bytecode (just now
> someone
> > could accidentally insert a new bytecode output between the source position
> > setting and the Call bytecode emission).
> > 
> > What set of bytecodes would we expect to need to set source positions on? Is
> it
> > a limited subset (like the current Call and CallNew), or could it be any
> > bytecode?
> > 
> > WDYT?
> 
> Eventually we would have sourcr position annotations at every expression and
> every statement, similar to FCG. You may be able to rule out some, but I think
> generally it could be any bytecode instruction. The current way to do it is
> least instrusive.
> 
> If we happen to insert a new instruction between the annotated code offset and
> the actual call instruction, I would argue that annotating that new
instruction
> is actually the correct thing to do. Source position calculation is not exact,
> but looks for the closest annotation anyways.

A bit to clarify: I would consider that new instruction between the annotated
code offset and the actual call instruction to be the correct annotation offset
because it, together with the call that follows it, constitute the AST node of
call.

[rmcilroy, 2016-02-05 10:05:54]

OK thanks for the explanation. I agree, this is less invasive than the
alternatives. LGTM.

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.cc:41: if (entries_.back().bytecode_offset
== offset) entries_.pop_back();
Should this be a while loop (e.g., could we try to add multiple source positions
for a given bytecode offset before trying to output a bytecode)?

[Yang, 2016-02-05 12:21:31]

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.cc:41: if (entries_.back().bytecode_offset
== offset) entries_.pop_back();
On 2016/02/05 10:05:54, rmcilroy wrote:
> Should this be a while loop (e.g., could we try to add multiple source
positions
> for a given bytecode offset before trying to output a bytecode)?

No. AssertMonotonic already makes sure that we do not set multiple source
positions for one byte code.

https://codereview.chromium.org/1668863002/diff/1/test/mjsunit/ignition/dead-...
File test/mjsunit/ignition/dead-code-source-position.js (right):

https://codereview.chromium.org/1668863002/diff/1/test/mjsunit/ignition/dead-...
test/mjsunit/ignition/dead-code-source-position.js:5: // Flags:
--ignition-filter=f
On 2016/02/04 15:06:39, rmcilroy wrote:
> nit - no need for --ignition-filter=f

Done.

[Yang, 2016-02-05 12:22:04]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-05 12:22:04]

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/1668863002/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-02-05 12:22:08]

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

[rmcilroy, 2016-02-05 12:24:31]

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.cc:41: if (entries_.back().bytecode_offset
== offset) entries_.pop_back();
On 2016/02/05 12:21:31, Yang wrote:
> On 2016/02/05 10:05:54, rmcilroy wrote:
> > Should this be a while loop (e.g., could we try to add multiple source
> positions
> > for a given bytecode offset before trying to output a bytecode)?
> 
> No. AssertMonotonic already makes sure that we do not set multiple source
> positions for one byte code.

Ahh, I was confused since Monotonic usually means a <= b, not a < b. I'm not
sure of a better name though.

[Yang, 2016-02-05 12:27:15]

On 2016/02/05 12:24:31, rmcilroy wrote:
>
https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
> File src/interpreter/source-position-table.cc (right):
> 
>
https://codereview.chromium.org/1668863002/diff/1/src/interpreter/source-posi...
> src/interpreter/source-position-table.cc:41: if
(entries_.back().bytecode_offset
> == offset) entries_.pop_back();
> On 2016/02/05 12:21:31, Yang wrote:
> > On 2016/02/05 10:05:54, rmcilroy wrote:
> > > Should this be a while loop (e.g., could we try to add multiple source
> > positions
> > > for a given bytecode offset before trying to output a bytecode)?
> > 
> > No. AssertMonotonic already makes sure that we do not set multiple source
> > positions for one byte code.
> 
> Ahh, I was confused since Monotonic usually means a <= b, not a < b. I'm not
> sure of a better name though.

Yeah... I could call it AssertStrictMonotonic but then I thought that would be
hairsplitting.

[commit-bot: I haz the power, 2016-02-05 12:37:42]

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

[commit-bot: I haz the power, 2016-02-05 12:37:42]

Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13366)
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/2539)
  v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Yang, 2016-02-05 12:52:03]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-05 12:52:03]

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/1668863002/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2016-02-05 12:52:08]

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

[commit-bot: I haz the power, 2016-02-05 13:29:06]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
[interpreter] source positions should not be emitted for dead code.

R=mstarzinger@chromium.org
==========

to

==========
[interpreter] source positions should not be emitted for dead code.

R=mstarzinger@chromium.org

Committed: https://crrev.com/85eff14c373e529d268945f9725af5b4350074e9
Cr-Commit-Position: refs/heads/master@{#33775}
==========

[commit-bot: I haz the power, 2016-02-05 13:29:53]

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