Refactor building of lazy bailouts in AstGraphBuilder.

Refactor building of lazy bailouts in AstGraphBuilder.

BUG=
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=23096

[Jarin, 2014-08-12 21:03:32]

As discussed in person, this refactors attaching of lazy bailouts to nodes in
the graph builder so that it allows adding framestates for debugger/arguments
object/inlining in future. 

Does this roughly correspond to what we agreed?

[Michael Starzinger, 2014-08-12 21:17:41]

LGTM. Yes, this corresponds to what we discussed very well. :)

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

https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:174: enum OutputFrameStateCombine {
PUSH_OUTPUT, IGNORE_OUTPUT };
nit: Can we add a comment describing the semantics behind this enum?

https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:176: void BuildLazyBailout(Node* node,
BailoutId ast_id,
nit: Since the BuildFoo methods all have a clear semantic (see comment above in
line 69), how would you feel about naming this AttachLazyBailout or
PrepareLazyBailout instead?

https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:178: OutputFrameStateCombine
StateCombineFromAstContext();
suggestion: I would be fine with moving this into AstContext so that one could
call "ast_context()->GetStateCombine()" instead (if that still makes all of them
fit into one line ^^).

[Michael Starzinger, 2014-08-12 21:27:40]

One more nit.

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

https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:177: OutputFrameStateCombine combine =
IGNORE_OUTPUT);
nit: Also, there seems to be no caller relying on the default argument. Can we
drop the default argument?

[Jarin, 2014-08-13 08:52:32]

On 2014/08/12 21:17:41, Michael Starzinger wrote:
> LGTM. Yes, this corresponds to what we discussed very well. :)
> 
>
https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
> File src/compiler/ast-graph-builder.h (right):
> 
>
https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:174: enum OutputFrameStateCombine {
> PUSH_OUTPUT, IGNORE_OUTPUT };
> nit: Can we add a comment describing the semantics behind this enum?
> 
>
https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:176: void BuildLazyBailout(Node* node,
> BailoutId ast_id,
> nit: Since the BuildFoo methods all have a clear semantic (see comment above
in
> line 69), how would you feel about naming this AttachLazyBailout or
> PrepareLazyBailout instead?
> 
>
https://codereview.chromium.org/464033002/diff/1/src/compiler/ast-graph-build...
> src/compiler/ast-graph-builder.h:178: OutputFrameStateCombine
> StateCombineFromAstContext();
> suggestion: I would be fine with moving this into AstContext so that one could
> call "ast_context()->GetStateCombine()" instead (if that still makes all of
them
> fit into one line ^^).

All this should be addressed now. Thanks!

[Michael Starzinger, 2014-08-13 09:15:58]

Still LGTM, just one more comment.

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

https://codereview.chromium.org/464033002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:185: OutputFrameStateCombine
StateCombineFromAstContext();
This method is no longer used or implemented, let's drop the declaration.

[Jarin, 2014-08-13 09:23:42]

Committed patchset #2 manually as 23096 (presubmit successful).