Make FrameStates recursive (to be used for inlining).

Make FrameStates recursive (to be used for inlining).

- Changes FrameState node to require a 5th input
  (use undefined as sentinel).
- Adds outer_state pointer to FrameStateDescriptor.
- Instruction selection dumps dependencies of FrameState chain into the instruction.
- Code generator builds the translation for rec. FrameStates.
  (will require further integration tests)

R=jarin@chromium.org

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

[sigurds, 2014-08-29 14:43:13]

jarin: PTAL, mstarzinger: FYI

[Jarin, 2014-08-29 15:07:07]

Nice. Just minor stuff...

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

https://codereview.chromium.org/517323002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:268: Node* Checkpoint(BailoutId ast_id, Node*
parent);
I believe we will always initially create framestates without parents, so we
should not add the argument here. Instead, the Checkpoint method should just
stick in the 'undefined' as the last input.

https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-build...
File src/compiler/graph-builder.cc (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-build...
src/compiler/graph-builder.cc:44: if (!has_context && !has_framestate &&
!has_control && !has_effect) {
Good catch! Thanks for fixing.

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector.cc:1083: if (descriptor->parent() != NULL) {
Could you recurse first? (So that the outer guys are first in the array.)

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
src/compiler/instruction.h:706: FrameStateDescriptor* parent = NULL)
Perhaps rename 'parent -> 'outer_state' (or 'outer'), so that it is clear which
way the nesting goes.

https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/...
File test/compiler-unittests/instruction-selector-unittest.cc (right):

https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/...
test/compiler-unittests/instruction-selector-unittest.cc:561: EXPECT_EQ(43,
s.ToInt32(call_instr->InputAt(2)));
Hmm, could we put the parent values first here?

[sigurds, 2014-09-01 08:48:18]

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

https://codereview.chromium.org/517323002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.h:268: Node* Checkpoint(BailoutId ast_id, Node*
parent);
On 2014/08/29 15:07:06, jarin wrote:
> I believe we will always initially create framestates without parents, so we
> should not add the argument here. Instead, the Checkpoint method should just
> stick in the 'undefined' as the last input.

Done.

https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-build...
File src/compiler/graph-builder.cc (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-build...
src/compiler/graph-builder.cc:44: if (!has_context && !has_framestate &&
!has_control && !has_effect) {
On 2014/08/29 15:07:07, jarin wrote:
> Good catch! Thanks for fixing.

Done.

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector.cc:1083: if (descriptor->parent() != NULL) {
On 2014/08/29 15:07:07, jarin wrote:
> Could you recurse first? (So that the outer guys are first in the array.)

Done.

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction...
src/compiler/instruction.h:706: FrameStateDescriptor* parent = NULL)
On 2014/08/29 15:07:07, jarin wrote:
> Perhaps rename 'parent -> 'outer_state' (or 'outer'), so that it is clear
which
> way the nesting goes.

Done.

https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/...
File test/compiler-unittests/instruction-selector-unittest.cc (right):

https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/...
test/compiler-unittests/instruction-selector-unittest.cc:561: EXPECT_EQ(43,
s.ToInt32(call_instr->InputAt(2)));
On 2014/08/29 15:07:07, jarin wrote:
> Hmm, could we put the parent values first here?

Done.

[sigurds, 2014-09-02 08:36:08]

PTAL again, after rebase on removal of lazydeopt/continuation blocks.

[Jarin, 2014-09-02 10:28:39]

lgtm with one reservation.

https://codereview.chromium.org/517323002/diff/100001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/517323002/diff/100001/src/compiler/code-gener...
src/compiler/code-generator.cc:313: descriptor->outer_state()->state_combine());
I think we will have to ignore the output for the outer guy. Just use
kIgnoreOutput here.

[sigurds, 2014-09-02 10:43:58]

FYI

https://codereview.chromium.org/517323002/diff/100001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/517323002/diff/100001/src/compiler/code-gener...
src/compiler/code-generator.cc:313: descriptor->outer_state()->state_combine());
On 2014/09/02 10:28:38, jarin wrote:
> I think we will have to ignore the output for the outer guy. Just use
> kIgnoreOutput here.

Done.

[sigurds, 2014-09-02 10:49:42]

Committed patchset #7 manually as 23595 (presubmit successful).