[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org

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

[Igor Sheludko, 2016-03-10 17:40:17]

Description was changed from

==========
[crankshaft] Support for ES6 tail calls inlining.

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

to

==========
[crankshaft] Support for ES6 tail calls inlining.

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

[Igor Sheludko, 2016-03-10 17:43:02]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2016-03-11 13:58:43]

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

[commit-bot: I haz the power, 2016-03-11 13:58:45]

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

[Igor Sheludko, 2016-03-11 13:58:59]

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

[Igor Sheludko, 2016-03-11 13:58:59]

PTAL

[commit-bot: I haz the power, 2016-03-11 14:14:37]

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

[commit-bot: I haz the power, 2016-03-11 14:14:38]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2798)
  v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Igor Sheludko, 2016-03-21 13:40:14]

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

[commit-bot: I haz the power, 2016-03-21 13:40:28]

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

[commit-bot: I haz the power, 2016-03-21 14:01:37]

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

[commit-bot: I haz the power, 2016-03-21 14:01:38]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[Michael Starzinger, 2016-03-21 14:06:50]

LGTM with nits.

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:13153: if (syntactic_tail_call_mode ==
TailCallMode::kAllow) {
Can we either add a DCHECK to the other {inlining_kind} cases that they are
never in syntactic tail-call positions? Or alternatively hoist this mutation of
the outer frame to happen before we start adding the stub frames (i.e. move it
to line 13137 above)?

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen.h
File src/crankshaft/hydrogen.h (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.h:685: void MarkAsTailCaller();
nit: Let's add a short comment explaining the semantics of this method (i.e. it
mutates this environment's type and potentially drops an outer environment from
the chain).

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/lithium.cc
File src/crankshaft/lithium.cc (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:530: hydrogen_env->frame_type() != JS_FUNCTION);
nit: I think this DCHECK can be made stronger ...

DCHECK(!ast_id.IsNone() ||
       (hydrogen_env->frame_type() != JS_FUNCTION &&
        hydrogen_env->frame_type() != TAIL_CALLER_FUNCTION));

[Igor Sheludko, 2016-03-22 13:47:18]

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

[commit-bot: I haz the power, 2016-03-22 13:47:28]

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

[Igor Sheludko, 2016-03-22 14:08:37]

Addressed comments and fixed the redness appeared in patch set 2. PTAL.

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:13120: outer_ = outer()->outer();
This was the source of try-bot redness. I'll drop the adaptor frame in lithium
instead.

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:13153: if (syntactic_tail_call_mode ==
TailCallMode::kAllow) {
On 2016/03/21 14:06:50, Michael Starzinger wrote:
> Can we either add a DCHECK to the other {inlining_kind} cases that they are
> never in syntactic tail-call positions? Or alternatively hoist this mutation
of
> the outer frame to happen before we start adding the stub frames (i.e. move it
> to line 13137 above)?

Done.

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen.h
File src/crankshaft/hydrogen.h (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.h:685: void MarkAsTailCaller();
On 2016/03/21 14:06:50, Michael Starzinger wrote:
> nit: Let's add a short comment explaining the semantics of this method (i.e.
it
> mutates this environment's type and potentially drops an outer environment
from
> the chain).

Done. It looks like we can't drop outer environment here. We have to do that in
lithium instead.

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/lithium.cc
File src/crankshaft/lithium.cc (right):

https://codereview.chromium.org/1782743003/diff/40001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:530: hydrogen_env->frame_type() != JS_FUNCTION);
On 2016/03/21 14:06:50, Michael Starzinger wrote:
> nit: I think this DCHECK can be made stronger ...
> 
> DCHECK(!ast_id.IsNone() ||
>        (hydrogen_env->frame_type() != JS_FUNCTION &&
>         hydrogen_env->frame_type() != TAIL_CALLER_FUNCTION));

Done.

[Michael Starzinger, 2016-03-22 14:12:51]

Yep, still LGTM. Thanks!

[Igor Sheludko, 2016-03-22 14:15:20]

Description was changed from

==========
[crankshaft] Support for ES6 tail calls inlining.

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

to

==========
[crankshaft] Support for ES6 tail calls inlining.

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

[Igor Sheludko, 2016-03-22 14:15:33]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2016-03-22 14:15:44]

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

[Igor Sheludko, 2016-03-22 14:20:20]

The CQ bit was unchecked by ishell@chromium.org

[Igor Sheludko, 2016-03-22 14:35:59]

Description was changed from

==========
[crankshaft] Support for ES6 tail calls inlining.

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[crankshaft] Support inlining of calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

[Igor Sheludko, 2016-03-22 14:41:10]

Description was changed from

==========
[crankshaft] Support inlining of calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

[Igor Sheludko, 2016-03-22 14:41:35]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2016-03-22 14:41:41]

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

[commit-bot: I haz the power, 2016-03-22 14:43:44]

Description was changed from

==========
[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

[commit-bot: I haz the power, 2016-03-22 14:43:45]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-22 14:44:38]

Description was changed from

==========
[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[crankshaft] Support inlining of function calls in tail position (in ES6 sense).

BUG=v8:4698
LOG=N
TBR=bmeurer@chromium.org

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

[commit-bot: I haz the power, 2016-03-22 14:44:39]

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

[Deon, 2016-05-20 02:51:38]

This change has a typo in " return AddInstruction(NewUncasted<I>(p1, p2, p3, p4,
p5, p6, p7, p8, p8));"
The last parameter should be p9.

On 2016/03/22 14:44:39, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/df694d552448bca451e4ffd406f5446bffb28fd2
> Cr-Commit-Position: refs/heads/master@{#34992}

[Igor Sheludko, 2016-05-20 05:49:09]

On 2016/05/20 02:51:38, Deon wrote:
> This change has a typo in " return AddInstruction(NewUncasted<I>(p1, p2, p3,
p4,
> p5, p6, p7, p8, p8));"
> The last parameter should be p9.
> 
> On 2016/03/22 14:44:39, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/df694d552448bca451e4ffd406f5446bffb28fd2
> > Cr-Commit-Position: refs/heads/master@{#34992}

Ouch! Thanks!
Luckily we don't use this function. The fix is on the way:
https://codereview.chromium.org/1998093002/