[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N

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

[Igor Sheludko, 2016-03-08 15:23:24]

Description was changed from

==========
[turbofan] [deoptimizer] Support inlining of calls at tail position in ES6
sense.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N
==========

to

==========
[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N
==========

[Igor Sheludko, 2016-03-08 15:32:11]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2016-03-08 15:32:22]

Patchset #1 (id:20001) has been deleted

[Igor Sheludko, 2016-03-08 15:43:00]

Patchset #1 (id:40001) has been deleted

[Igor Sheludko, 2016-03-08 15:43:08]

Patchset #1 (id:60001) has been deleted

[Igor Sheludko, 2016-03-08 15:50:38]

Patchset #1 (id:80001) has been deleted

[Igor Sheludko, 2016-03-08 15:52:47]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 15:52:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768263004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768263004/100001

[commit-bot: I haz the power, 2016-03-08 15:54:02]

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

[commit-bot: I haz the power, 2016-03-08 15:54:03]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2549)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14948)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/2547)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Igor Sheludko, 2016-03-08 15:54:20]

ishell@chromium.org changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org

[Igor Sheludko, 2016-03-08 15:54:21]

Michi and Jaro, PTAL

[Igor Sheludko, 2016-03-08 16:07:00]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 16:07:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768263004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768263004/140001

[Michael Starzinger, 2016-03-08 16:25:08]

Approach is looking good. Here is a first round of comments.

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
File test/mjsunit/es6/tail-call-megatest.js (right):

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
test/mjsunit/es6/tail-call-megatest.js:7: // Flags: --turbo --nostress-opt
Is the "--turbo" flag required here? One of the variants we run already contains
the flag. That should be sufficient AFAICT.

https://codereview.chromium.org/1768263004/diff/140001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1768263004/diff/140001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:523: if (node->opcode() ==
IrOpcode::kJSCallFunction) {
This definitely needs a comment explaining the reasoning behind this artificial
frame and what its effects on the deoptimizer are. Also can we move it down one
block to right before the arguments adaptor frame is created (i.e. to line 546
below).

https://codereview.chromium.org/1768263004/diff/140001/src/deoptimizer.h
File src/deoptimizer.h (right):

https://codereview.chromium.org/1768263004/diff/140001/src/deoptimizer.h#newc...
src/deoptimizer.h:952: V(JS_TAIL_CALLER_FRAME)          \
nit: Can we call this "TAIL_CALLER_FRAME" without the JS prefix? For symmetry
and also because it is just a marker frame without JS semantics.

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (left):

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:127: DCHECK(frame->function() == *function);
nit: I understand why this DCHECK no longer holds, and AFAICT the code below
should still be correct even without this invariant holding. But I would feel
safer if we would just drop the "frame" variable and the RUNTIME_ASSERT one line
above as well. Because having those two lines might be misleading.

We could even go as far as adding a comment below (see next comment).

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:138: // Search for other activations of the same
function and code.
This comment is outdated, we only search for "activations of the same code", the
function is ignored. Also I would add a comment similar to the following here
...

"At this point {it} is at the topmost frame of all the frames materialized by
the deoptimizer. Note that this frame does not necessarily represent and
activation of {function} because of potential inlined tail-calls."

[commit-bot: I haz the power, 2016-03-08 16:28:55]

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

[commit-bot: I haz the power, 2016-03-08 16:28:55]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-03-08 18:03:40]

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
File test/mjsunit/es6/tail-call-megatest.js (right):

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
test/mjsunit/es6/tail-call-megatest.js:7: // Flags: --turbo --nostress-opt
On 2016/03/08 16:25:08, Michael Starzinger wrote:
> Is the "--turbo" flag required here? One of the variants we run already
contains
> the flag. That should be sufficient AFAICT.

It is here to avoid running this test with Crankshaft untill it supports tail
calls inlining.

https://codereview.chromium.org/1768263004/diff/140001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1768263004/diff/140001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:523: if (node->opcode() ==
IrOpcode::kJSCallFunction) {
On 2016/03/08 16:25:08, Michael Starzinger wrote:
> This definitely needs a comment explaining the reasoning behind this
artificial
> frame and what its effects on the deoptimizer are. Also can we move it down
one
> block to right before the arguments adaptor frame is created (i.e. to line 546
> below).

Done. Please review the comment.

https://codereview.chromium.org/1768263004/diff/140001/src/deoptimizer.h
File src/deoptimizer.h (right):

https://codereview.chromium.org/1768263004/diff/140001/src/deoptimizer.h#newc...
src/deoptimizer.h:952: V(JS_TAIL_CALLER_FRAME)          \
On 2016/03/08 16:25:08, Michael Starzinger wrote:
> nit: Can we call this "TAIL_CALLER_FRAME" without the JS prefix? For symmetry
> and also because it is just a marker frame without JS semantics.

Done.

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (left):

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:127: DCHECK(frame->function() == *function);
On 2016/03/08 16:25:08, Michael Starzinger wrote:
> nit: I understand why this DCHECK no longer holds, and AFAICT the code below
> should still be correct even without this invariant holding. But I would feel
> safer if we would just drop the "frame" variable and the RUNTIME_ASSERT one
line
> above as well. Because having those two lines might be misleading.
> 
> We could even go as far as adding a comment below (see next comment).

Done.

https://codereview.chromium.org/1768263004/diff/140001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:138: // Search for other activations of the same
function and code.
On 2016/03/08 16:25:08, Michael Starzinger wrote:
> This comment is outdated, we only search for "activations of the same code",
the
> function is ignored. Also I would add a comment similar to the following here
> ...
> 
> "At this point {it} is at the topmost frame of all the frames materialized by
> the deoptimizer. Note that this frame does not necessarily represent and
> activation of {function} because of potential inlined tail-calls."

Done. Thanks for suggestion!

[Igor Sheludko, 2016-03-08 18:03:46]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 18:03:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768263004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768263004/160001

[commit-bot: I haz the power, 2016-03-08 18:25:20]

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

[commit-bot: I haz the power, 2016-03-08 18:25:21]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-03-08 21:29:53]

LGTM on the compiler and the plumbing. I didn't look at the deoptimizer, please
wait on Jaro to take a look at that.

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
File test/mjsunit/es6/tail-call-megatest.js (right):

https://codereview.chromium.org/1768263004/diff/120001/test/mjsunit/es6/tail-...
test/mjsunit/es6/tail-call-megatest.js:7: // Flags: --turbo --nostress-opt
On 2016/03/08 18:03:40, Igor Sheludko wrote:
> On 2016/03/08 16:25:08, Michael Starzinger wrote:
> > Is the "--turbo" flag required here? One of the variants we run already
> contains
> > the flag. That should be sufficient AFAICT.
> 
> It is here to avoid running this test with Crankshaft untill it supports tail
> calls inlining.

Acknowledged. If it's temporary then I don't mind.

[Jarin, 2016-03-09 10:40:07]

Awesome! lgtm.

https://codereview.chromium.org/1768263004/diff/160001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1768263004/diff/160001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:545: // will not "see" caller's arguments). Another
important thing is that
I do not entirely understand this comment - is not the caller's adaptor frame
stripped?

(However, I do understand there needs to be a flag for the deoptimizer to strip
the arguments adaptor for the top frame.)

[Igor Sheludko, 2016-03-09 10:44:14]

Patchset #5 (id:180001) has been deleted

[Igor Sheludko, 2016-03-09 10:44:55]

Thanks! Landing...

https://codereview.chromium.org/1768263004/diff/160001/src/compiler/js-inlini...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1768263004/diff/160001/src/compiler/js-inlini...
src/compiler/js-inlining.cc:545: // will not "see" caller's arguments). Another
important thing is that
On 2016/03/09 10:40:07, Jarin wrote:
> I do not entirely understand this comment - is not the caller's adaptor frame
> stripped?
> 
> (However, I do understand there needs to be a flag for the deoptimizer to
strip
> the arguments adaptor for the top frame.)

Done.

[Igor Sheludko, 2016-03-09 10:45:10]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 10:45:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768263004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768263004/200001

[commit-bot: I haz the power, 2016-03-09 11:08:50]

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

[commit-bot: I haz the power, 2016-03-09 11:08:51]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-03-09 11:31:13]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2016-03-09 11:31:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1768263004/#ps200001
(title: "Addressing comments, StandardFrameConstants -> CommonFrameConstants")

[commit-bot: I haz the power, 2016-03-09 11:31:20]

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

[commit-bot: I haz the power, 2016-03-09 11:33:19]

Description was changed from

==========
[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N
==========

to

==========
[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N
==========

[commit-bot: I haz the power, 2016-03-09 11:33:20]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2016-03-09 11:35:43]

Description was changed from

==========
[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N
==========

to

==========
[turbofan] [deoptimizer] Support inlining of ES6 tail calls.

In case when F was called with incompatible number of arguments (and therefore
the arguments adator frame was created), F inlines a tail call of G which then
deopts the deoptimizer should also remove the arguments adaptor frame for F.

This CL adds required machinery to the deoptimizer.

BUG=v8:4698
LOG=N

Committed: https://crrev.com/c29a4560bb548fa0ebeec8262f9d6fca9d50fe7a
Cr-Commit-Position: refs/heads/master@{#34610}
==========

[commit-bot: I haz the power, 2016-03-09 11:35:44]

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