[turbofan]: Improved source position information

[turbofan]: Improved source position information

Make sure the initial graph is fully populated with source position information and automatically propagate that information down through newly allocated nodes during reduction passes in the most unobtrusive way that's currently possible.

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

[danno, 2015-02-04 13:17:36]

danno@chromium.org changed reviewers:
+ mstarzinger@chromium.org, titzer@chromium.org

[danno, 2015-02-04 13:17:37]

PTAL

[titzer, 2015-02-04 13:23:44]

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
src/compiler/simplified-lowering.cc:98: SourcePositionTable::Scope scope(
You shouldn't need the scope during the propagation phase.

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
src/compiler/simplified-lowering.cc:462: void VisitNode(Node* node,
MachineTypeUnion use,
I'd rather see this inlined into the second loop (or two loop versions) in the
Run() method, since it's not needed during propagation.

[danno, 2015-02-04 13:42:18]

Please take another look

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
src/compiler/simplified-lowering.cc:98: SourcePositionTable::Scope scope(
On 2015/02/04 13:23:44, titzer wrote:
> You shouldn't need the scope during the propagation phase.

Done.

https://codereview.chromium.org/897883002/diff/40001/src/compiler/simplified-...
src/compiler/simplified-lowering.cc:462: void VisitNode(Node* node,
MachineTypeUnion use,
On 2015/02/04 13:23:44, titzer wrote:
> I'd rather see this inlined into the second loop (or two loop versions) in the
> Run() method, since it's not needed during propagation.

Done.

[titzer, 2015-02-04 13:46:19]

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.h
File src/compiler/graph.h (right):

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.h#new...
src/compiler/graph.h:85: void Decorate(Node* node, bool incomplete);
Is this incomplete flag necessary for source positions? Seems like there is an
unrelated change to the typer included in this CL.

[Michael Starzinger, 2015-02-04 13:54:03]

LGTM with comments. I looked at everything outside of simplified lowering, I'll
leave that to Ben.

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.cc
File src/compiler/graph.cc (right):

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.cc#ne...
src/compiler/graph.cc:27: decorator->Decorate(node, incomplete);
nit: Can we put curly braces around the body?

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

https://codereview.chromium.org/897883002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:349: SourcePositionWrapper* wrapper = new
(data->graph_zone())
According to Benedikt we should use the existing placement operator instead of
overriding it ...

void* buffer = zone->New(sizeof(SourcePositionWrapper));
SourcePositionWrapper* wrapper =
    new(buffer) SourcePositionWrapper(reducer, data->source_positions());

https://codereview.chromium.org/897883002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:356: };
nit: Can we add "// namespace", also semi-colon seems redundant.

[danno, 2015-02-04 14:02:30]

New patchsets have been uploaded after l-g-t-m from mstarzinger@chromium.org

[danno, 2015-02-04 14:05:00]

Please take another look

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.cc
File src/compiler/graph.cc (right):

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.cc#ne...
src/compiler/graph.cc:27: decorator->Decorate(node, incomplete);
On 2015/02/04 13:54:03, Michael Starzinger wrote:
> nit: Can we put curly braces around the body?

Done.

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.h
File src/compiler/graph.h (right):

https://codereview.chromium.org/897883002/diff/60001/src/compiler/graph.h#new...
src/compiler/graph.h:85: void Decorate(Node* node, bool incomplete);
On 2015/02/04 13:46:19, titzer wrote:
> Is this incomplete flag necessary for source positions? Seems like there is an
> unrelated change to the typer included in this CL.

Nope, this is intended. Note that previously Decorators were not called when
nodes were incomplete. This was because the typer didn't want to type some
"incomplete" nodes. However, the source position decorator _always_ wants to be
invoked, even for incomplete nodes.

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

https://codereview.chromium.org/897883002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:349: SourcePositionWrapper* wrapper = new
(data->graph_zone())
On 2015/02/04 13:54:03, Michael Starzinger wrote:
> According to Benedikt we should use the existing placement operator instead of
> overriding it ...
> 
> void* buffer = zone->New(sizeof(SourcePositionWrapper));
> SourcePositionWrapper* wrapper =
>     new(buffer) SourcePositionWrapper(reducer, data->source_positions());

Done.

https://codereview.chromium.org/897883002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:356: };
On 2015/02/04 13:54:03, Michael Starzinger wrote:
> nit: Can we add "// namespace", also semi-colon seems redundant.

Done.

[titzer, 2015-02-05 12:45:58]

lgtm

[danno, 2015-02-05 12:52:11]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-02-05 12:52:19]

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

[commit-bot: I haz the power, 2015-02-05 13:16:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-05 13:17:05]

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