[wasm] Generate source position information

[wasm] Generate source position information

Annotate call nodes in the TF graph with source code information in the form
of byte offset relative to the wasm function start. The backend finally outputs those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org

Committed: https://crrev.com/91386f0bc09b47dfe61ad5779d23f978585e9519
Cr-Commit-Position: refs/heads/master@{#35793}

[Clemens Hammacher, 2016-04-14 10:42:22]

I know that I am doing some questionable changes, so please provide feedback as
appropriate.
I manually checked that the relocation info is created, but I will only land the
CL once I also have tests for it.

[Clemens Hammacher, 2016-04-14 12:45:35]

Patch 2 does not include compiler-internal stuff any more to the ast decoder.
In order to do this, I had to make SourcePositionTable::Scope non-nested (now
SourcePositionScope) such that it can be forward-referenced.

[Clemens Hammacher, 2016-04-14 13:58:11]

Description was changed from

==========
[wasm] Generate source position information

During TF graph construction, the wasm byte offset relative to function
start is tracked for each node, and finally output as RelocInfo.
Tests will follow.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
BUG=
==========

to

==========
[wasm] Generate source position information                              
                                                                         
During TF graph construction, the wasm byte offset relative to function  
start is tracked for each node, and finally output as RelocInfo.         
Convert SourcePositionTable::Scope to non-nested class, such that it        
can be forward-referenced.                                               
                                                                         
Tests will follow.                                                       

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
BUG=
==========

[titzer, 2016-04-18 11:03:51]

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h#newcode464
src/compiler.h:464: source_position_table_ = table;
I'm not sure that the CompilationInfo is the best place to dump this, since it's
a compiler-internal field, but I don't have a better suggestion off the top of
my head.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
File src/compiler/source-position.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
src/compiler/source-position.h:54: #ifdef DEBUG
I'm not sure why this would only be useful in debug mode.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
src/compiler/source-position.h:72: class SourcePositionScope final {
I think I prefer leaving this as an inner class in SourcePositionTable.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:153: // Register a graph decorator to assign source
positions to nodes (in
I don't think we should add scoping for this. I think it'd be better to pass the
source position explicitly to the methods that build nodes, for exactly those
nodes that need positions. Those are binops (because division can trap) and
calls.

[Michael Starzinger, 2016-04-18 11:08:47]

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h#newcode464
src/compiler.h:464: source_position_table_ = table;
On 2016/04/18 11:03:51, titzer wrote:
> I'm not sure that the CompilationInfo is the best place to dump this, since
it's
> a compiler-internal field, but I don't have a better suggestion off the top of
> my head.

Not a fan of adding this to CompilationInfo either. IIUC, this is only needed to
pass the source position table from the WASM compiler into the TurboFan
pipeline. If we were to add a dedicated Pipeline::GenerateWASMCode, then we
could it pass in thorugh exactly that channel. This would also finally get rid
of the call to GenerateCodeForTesting from production code. WDYT?

[titzer, 2016-04-18 11:14:25]

On 2016/04/18 11:08:47, Michael Starzinger wrote:
> https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h
> File src/compiler.h (right):
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h#newcode464
> src/compiler.h:464: source_position_table_ = table;
> On 2016/04/18 11:03:51, titzer wrote:
> > I'm not sure that the CompilationInfo is the best place to dump this, since
> it's
> > a compiler-internal field, but I don't have a better suggestion off the top
of
> > my head.
> 
> Not a fan of adding this to CompilationInfo either. IIUC, this is only needed
to
> pass the source position table from the WASM compiler into the TurboFan
> pipeline. If we were to add a dedicated Pipeline::GenerateWASMCode, then we
> could it pass in thorugh exactly that channel. This would also finally get rid
> of the call to GenerateCodeForTesting from production code. WDYT?

SGTM

[Clemens Hammacher, 2016-04-18 11:18:10]

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h#newcode464
src/compiler.h:464: source_position_table_ = table;
On 2016/04/18 at 11:08:47, Michael Starzinger wrote:
> On 2016/04/18 11:03:51, titzer wrote:
> > I'm not sure that the CompilationInfo is the best place to dump this, since
it's
> > a compiler-internal field, but I don't have a better suggestion off the top
of
> > my head.
> 
> Not a fan of adding this to CompilationInfo either. IIUC, this is only needed
to pass the source position table from the WASM compiler into the TurboFan
pipeline. If we were to add a dedicated Pipeline::GenerateWASMCode, then we
could it pass in thorugh exactly that channel. This would also finally get rid
of the call to GenerateCodeForTesting from production code. WDYT?

Sounds reasonable. I will try to change it that way.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
File src/compiler/source-position.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
src/compiler/source-position.h:54: #ifdef DEBUG
On 2016/04/18 at 11:03:51, titzer wrote:
> I'm not sure why this would only be useful in debug mode.

Right now it's only used in debug mode, but sure, it does not hurt to make it
always available.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
src/compiler/source-position.h:72: class SourcePositionScope final {
On 2016/04/18 at 11:03:51, titzer wrote:
> I think I prefer leaving this as an inner class in SourcePositionTable.

If we don't need to forward-reference it from compiler.h, then I will revert
this change.

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:153: // Register a graph decorator to assign source
positions to nodes (in
On 2016/04/18 at 11:03:51, titzer wrote:
> I don't think we should add scoping for this. I think it'd be better to pass
the source position explicitly to the methods that build nodes, for exactly
those nodes that need positions. Those are binops (because division can trap)
and calls.

What about step-through debugging? Wouldn't that require source position
information on basically each single node?

[titzer, 2016-04-18 11:21:57]

On 2016/04/18 11:18:10, Clemens Hammacher wrote:
> https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h
> File src/compiler.h (right):
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler.h#newcode464
> src/compiler.h:464: source_position_table_ = table;
> On 2016/04/18 at 11:08:47, Michael Starzinger wrote:
> > On 2016/04/18 11:03:51, titzer wrote:
> > > I'm not sure that the CompilationInfo is the best place to dump this,
since
> it's
> > > a compiler-internal field, but I don't have a better suggestion off the
top
> of
> > > my head.
> > 
> > Not a fan of adding this to CompilationInfo either. IIUC, this is only
needed
> to pass the source position table from the WASM compiler into the TurboFan
> pipeline. If we were to add a dedicated Pipeline::GenerateWASMCode, then we
> could it pass in thorugh exactly that channel. This would also finally get rid
> of the call to GenerateCodeForTesting from production code. WDYT?
> 
> Sounds reasonable. I will try to change it that way.
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
> File src/compiler/source-position.h (right):
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
> src/compiler/source-position.h:54: #ifdef DEBUG
> On 2016/04/18 at 11:03:51, titzer wrote:
> > I'm not sure why this would only be useful in debug mode.
> 
> Right now it's only used in debug mode, but sure, it does not hurt to make it
> always available.
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler/source-pos...
> src/compiler/source-position.h:72: class SourcePositionScope final {
> On 2016/04/18 at 11:03:51, titzer wrote:
> > I think I prefer leaving this as an inner class in SourcePositionTable.
> 
> If we don't need to forward-reference it from compiler.h, then I will revert
> this change.
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
> File src/compiler/wasm-compiler.h (right):
> 
>
https://codereview.chromium.org/1890803002/diff/40001/src/compiler/wasm-compi...
> src/compiler/wasm-compiler.h:153: // Register a graph decorator to assign
source
> positions to nodes (in
> On 2016/04/18 at 11:03:51, titzer wrote:
> > I don't think we should add scoping for this. I think it'd be better to pass
> the source position explicitly to the methods that build nodes, for exactly
> those nodes that need positions. Those are binops (because division can trap)
> and calls.
> 
> What about step-through debugging? Wouldn't that require source position
> information on basically each single node?

We will use the interpreter for that, not the optimized code.

[Clemens Hammacher, 2016-04-22 09:23:01]

Description was changed from

==========
[wasm] Generate source position information                              
                                                                         
During TF graph construction, the wasm byte offset relative to function  
start is tracked for each node, and finally output as RelocInfo.         
Convert SourcePositionTable::Scope to non-nested class, such that it        
can be forward-referenced.                                               
                                                                         
Tests will follow.                                                       

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
BUG=
==========

to

==========
[wasm] Generate source position information

The Pipeline now has it's own GenerateWASMCode, which gets a SourcePositionTable
as argument. During TF graph construction, certain wasm opcodes (calls, trapping
binops, memory operations) store the source position (as byte offset) for the
respective node. The backend finally outputs those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-22 09:54:22]

PTAL.

[Michael Starzinger, 2016-04-22 11:30:21]

LGTM from my end with nits.

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/pipeline.h...
src/compiler/pipeline.h:37: // code.
nit: We could even add "(specifically modeled for the WASM compiler)" to the
comment.

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
File src/compiler/source-position.h (right):

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
src/compiler/source-position.h:77: 
nit: Drop empty newline between getter and setter for readability.

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
src/compiler/source-position.h:80: Graph* GetGraph() const { return graph_; }
nit: s/GetGraph/graph/ for this simple accessor. Also AFAICT the getter is only
needed for a DCHECK. I cannot judge the importance of the DCHECK, but if its
importance is negligible then I would vote for also dropping this getter.

[Clemens Hammacher, 2016-04-22 13:33:53]

Description was changed from

==========
[wasm] Generate source position information

The Pipeline now has it's own GenerateWASMCode, which gets a SourcePositionTable
as argument. During TF graph construction, certain wasm opcodes (calls, trapping
binops, memory operations) store the source position (as byte offset) for the
respective node. The backend finally outputs those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

to

==========
[wasm] Generate source position information

The Pipeline now has it's own GenerateWASMCode, which gets a SourcePositionTable
as argument. During TF graph construction, certain wasm opcodes (calls, trapping
binops, memory operations) store the source position (as byte offset) for the
respective node. The backend finally outputs those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-22 13:46:40]

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
File src/compiler/source-position.h (right):

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
src/compiler/source-position.h:77: 
On 2016/04/22 at 11:30:21, Michael Starzinger wrote:
> nit: Drop empty newline between getter and setter for readability.

It's already there (getter on line 76, setter on 78)? ;)

https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
src/compiler/source-position.h:80: Graph* GetGraph() const { return graph_; }
On 2016/04/22 at 11:30:21, Michael Starzinger wrote:
> nit: s/GetGraph/graph/ for this simple accessor. Also AFAICT the getter is
only needed for a DCHECK. I cannot judge the importance of the DCHECK, but if
its importance is negligible then I would vote for also dropping this getter.

OK, dropped it.

[Clemens Hammacher, 2016-04-25 16:12:59]

On 2016/04/22 at 13:46:40, Clemens Hammacher wrote:
>
https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
> File src/compiler/source-position.h (right):
> 
>
https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
> src/compiler/source-position.h:77: 
> On 2016/04/22 at 11:30:21, Michael Starzinger wrote:
> > nit: Drop empty newline between getter and setter for readability.
> 
> It's already there (getter on line 76, setter on 78)? ;)

I got it now, and *dropped* the newline.

Also, this is now rebased on your change of allocating the SourcePositionTable
in the Zone.

[Michael Starzinger, 2016-04-25 17:36:37]

On 2016/04/25 16:12:59, Clemens Hammacher wrote:
> On 2016/04/22 at 13:46:40, Clemens Hammacher wrote:
> >
>
https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
> > File src/compiler/source-position.h (right):
> > 
> >
>
https://codereview.chromium.org/1890803002/diff/80001/src/compiler/source-pos...
> > src/compiler/source-position.h:77: 
> > On 2016/04/22 at 11:30:21, Michael Starzinger wrote:
> > > nit: Drop empty newline between getter and setter for readability.
> > 
> > It's already there (getter on line 76, setter on 78)? ;)
> 
> I got it now, and *dropped* the newline.
> 
> Also, this is now rebased on your change of allocating the SourcePositionTable
> in the Zone.

Ack. Thanks! Yep, still LGTM from my end.

[Michael Starzinger, 2016-04-25 17:42:18]

https://codereview.chromium.org/1890803002/diff/140001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1890803002/diff/140001/src/compiler/pipeline....
src/compiler/pipeline.cc:1299: pipeline_statistics->BeginPhaseKind("test
codegen");
nit: s/test codegen/WASM codegen/ here.

https://codereview.chromium.org/1890803002/diff/140001/src/compiler/pipeline....
src/compiler/pipeline.cc:1304: if (data.schedule() == nullptr) {
nit: Guaranteed to always be null for WASM.

[Clemens Hammacher, 2016-04-26 10:21:22]

Description was changed from

==========
[wasm] Generate source position information

The Pipeline now has it's own GenerateWASMCode, which gets a SourcePositionTable
as argument. During TF graph construction, certain wasm opcodes (calls, trapping
binops, memory operations) store the source position (as byte offset) for the
respective node. The backend finally outputs those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

to

==========
[wasm] Generate source position information

Annotate call nodes in the TF graph with source code information in the form
of byte offset relative to the wasm function start. The backend finally outputs
those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

[Clemens Hammacher, 2016-04-26 12:16:19]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-04-26 12:16:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1890803002/#ps220001
(title: "avoid using std::tie")

[commit-bot: I haz the power, 2016-04-26 12:16:21]

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

[commit-bot: I haz the power, 2016-04-26 12:20:12]

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

[commit-bot: I haz the power, 2016-04-26 12:20:13]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/14158)

[Clemens Hammacher, 2016-04-26 12:34:51]

On 2016/04/26 at 12:20:13, commit-bot wrote:
> Try jobs failed on following builders:
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/14158)

Still missing LGTM from titzer :(

[titzer, 2016-04-26 12:42:54]

On 2016/04/26 12:34:51, Clemens Hammacher wrote:
> On 2016/04/26 at 12:20:13, commit-bot wrote:
> > Try jobs failed on following builders:
> >   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/14158)
> 
> Still missing LGTM from titzer :(

lgtm

[Clemens Hammacher, 2016-04-26 12:43:23]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2016-04-26 12:43:27]

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

[commit-bot: I haz the power, 2016-04-26 12:46:08]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-26 12:47:22]

Description was changed from

==========
[wasm] Generate source position information

Annotate call nodes in the TF graph with source code information in the form
of byte offset relative to the wasm function start. The backend finally outputs
those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
==========

to

==========
[wasm] Generate source position information

Annotate call nodes in the TF graph with source code information in the form
of byte offset relative to the wasm function start. The backend finally outputs
those positions as RelocInfo.

R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org

Committed: https://crrev.com/91386f0bc09b47dfe61ad5779d23f978585e9519
Cr-Commit-Position: refs/heads/master@{#35793}
==========

[commit-bot: I haz the power, 2016-04-26 12:47:23]

Patchset 12 (id:??) landed as
https://crrev.com/91386f0bc09b47dfe61ad5779d23f978585e9519
Cr-Commit-Position: refs/heads/master@{#35793}