[turbofan] Put StructuredGraphBuilder out of its misery and merge its remnants back into the AstGra…

[turbofan] Put StructuredGraphBuilder out of its misery and merge its remnants back into the AstGraphBuilder.

R=mstarzinger@chromium.org
BUG=

Committed: https://crrev.com/2379d34bdcfbf59a8e36bec69bc16ab9694ec019
Cr-Commit-Position: refs/heads/master@{#26387}

[Michael Starzinger, 2015-02-02 18:08:24]

I freakin love this change! Thank you!

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:531: AstGraphBuilder::Environment* env =
environment()->CopyAsUnreachable();
nit: Just drop the prefix.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:538: AstGraphBuilder::Environment* env =
environment()->CopyAsUnreachable();
nit: Just drop the prefix.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:99: Node* MakeNode(const Operator* op, int
value_input_count, Node** value_inputs,
Can we try to make these methods protected again. Ideally only CreateGraph()
would be public.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:198: LoopAssignmentAnalysis*
loop_assignment_analysis_;
nit: Can we move the loop_assignment_analysis down again and bring back the
original comment?

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:202: Zone* local_zone() const { return
local_zone_; }
nit: Can we move the getter down to the others (near line 246) for readability?

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:252: Node* current_context() const { return
current_context_; }
nit: Can we move this block of accessors (all six of them) up to the
undocumented block (i.e. up three lines) for readability?

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:429: Node* GetContext() { return
builder_->current_context(); }
nit: This seems to only have one use, can we inline it at the use-site in
Checkpoint() and get rid of this helper?

https://codereview.chromium.org/894073002/diff/1/test/cctest/compiler/graph-b...
File test/cctest/compiler/graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/1/test/cctest/compiler/graph-b...
test/cctest/compiler/graph-builder.h:6: #define V8_COMPILER_GRAPH_BUILDER_H_
This entire file looks like a remnant.

[titzer, 2015-02-02 18:26:48]

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:531: AstGraphBuilder::Environment* env =
environment()->CopyAsUnreachable();
On 2015/02/02 18:08:23, Michael Starzinger wrote:
> nit: Just drop the prefix.

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:538: AstGraphBuilder::Environment* env =
environment()->CopyAsUnreachable();
On 2015/02/02 18:08:23, Michael Starzinger wrote:
> nit: Just drop the prefix.

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:99: Node* MakeNode(const Operator* op, int
value_input_count, Node** value_inputs,
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> Can we try to make these methods protected again. Ideally only CreateGraph()
> would be public.

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:198: LoopAssignmentAnalysis*
loop_assignment_analysis_;
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> nit: Can we move the loop_assignment_analysis down again and bring back the
> original comment?

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:202: Zone* local_zone() const { return
local_zone_; }
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> nit: Can we move the getter down to the others (near line 246) for
readability?

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:252: Node* current_context() const { return
current_context_; }
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> nit: Can we move this block of accessors (all six of them) up to the
> undocumented block (i.e. up three lines) for readability?

Done.

https://codereview.chromium.org/894073002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:429: Node* GetContext() { return
builder_->current_context(); }
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> nit: This seems to only have one use, can we inline it at the use-site in
> Checkpoint() and get rid of this helper?

Done.

https://codereview.chromium.org/894073002/diff/1/test/cctest/compiler/graph-b...
File test/cctest/compiler/graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/1/test/cctest/compiler/graph-b...
test/cctest/compiler/graph-builder.h:6: #define V8_COMPILER_GRAPH_BUILDER_H_
On 2015/02/02 18:08:24, Michael Starzinger wrote:
> This entire file looks like a remnant.

Done.

[Michael Starzinger, 2015-02-02 18:34:55]

LGTM. I love it.

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:39: Node* GetFunctionContext();
nit: This can be made protected I think.

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:189: Node* GetFunctionClosure();
nit: Can we keep this one together with GetFunctionContext() and make it
protected as well?

[titzer, 2015-02-02 18:39:06]

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:39: Node* GetFunctionContext();
On 2015/02/02 18:34:55, Michael Starzinger wrote:
> nit: This can be made protected I think.

Done.

https://codereview.chromium.org/894073002/diff/40001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:189: Node* GetFunctionClosure();
On 2015/02/02 18:34:55, Michael Starzinger wrote:
> nit: Can we keep this one together with GetFunctionContext() and make it
> protected as well?

Done.

[titzer, 2015-02-02 18:39:10]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2015-02-02 18:39:53]

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

[commit-bot: I haz the power, 2015-02-02 19:09:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-02 19:10:13]

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