[es6] Properly handle the case when an inlined getter/setter/constructor does a tail call.

[es6] Properly handle the case when an inlined getter/setter/constructor does a tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

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

[Igor Sheludko, 2016-05-02 11:26:40]

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

[commit-bot: I haz the power, 2016-05-02 11:26:42]

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

[Igor Sheludko, 2016-05-02 11:27:49]

Description was changed from

==========
[es6] Don't alter outer stub environment when inlining getter/setter/constructor
that does a tail call.

BUG=chromium:608278
LOG=N
==========

to

==========
[es6] Don't alter outer stub environment when inlining getter/setter/constructor
that does a tail call.

BUG=chromium:608278
LOG=N
TBR=bmeurer@chromium.org
==========

[Igor Sheludko, 2016-05-02 11:28:07]

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

[Igor Sheludko, 2016-05-02 11:28:08]

PTAL

[commit-bot: I haz the power, 2016-05-02 11:41:12]

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

[commit-bot: I haz the power, 2016-05-02 11:41:13]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Igor Sheludko, 2016-05-02 11:43:13]

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

[commit-bot: I haz the power, 2016-05-02 11:43:22]

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

[commit-bot: I haz the power, 2016-05-02 11:57:52]

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

[commit-bot: I haz the power, 2016-05-02 11:57:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Igor Sheludko, 2016-05-03 09:10:25]

Description was changed from

==========
[es6] Don't alter outer stub environment when inlining getter/setter/constructor
that does a tail call.

BUG=chromium:608278
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

BUG=chromium:608278
LOG=N
TBR=bmeurer@chromium.org
==========

[Igor Sheludko, 2016-05-03 09:14:14]

Patchset #3 (id:40001) has been deleted

[Igor Sheludko, 2016-05-03 09:14:38]

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

[commit-bot: I haz the power, 2016-05-03 09:14:45]

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

[Igor Sheludko, 2016-05-03 09:27:11]

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

[Igor Sheludko, 2016-05-03 09:27:11]

Michi, PTAL Crankshaft.
Jaro, PTAL deoptimizer.

[Igor Sheludko, 2016-05-03 09:38:16]

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

https://codereview.chromium.org/1936043002/diff/60001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:529: if (hydrogen_env->frame_type() == JS_FUNCTION) {
Note: this is a modification to the old code.

[commit-bot: I haz the power, 2016-05-03 09:44:59]

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

[commit-bot: I haz the power, 2016-05-03 09:45:00]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-05-03 10:36:31]

Description was changed from

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

BUG=chromium:608278
LOG=N
TBR=bmeurer@chromium.org
==========

to

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

[Jarin, 2016-05-03 14:42:36]

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

https://codereview.chromium.org/1936043002/diff/60001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:529: if (hydrogen_env->frame_type() == JS_FUNCTION) {
On 2016/05/03 09:38:16, Igor Sheludko wrote:
> Note: this is a modification to the old code.

Could you explain why the getters do not need to push here?

Would it makes sense to add an else clause that would DCHECK the expected
frame_types; something like

} else {
  DCHECK(hydrogen_env->frame_type() == GETTER || hydrogen_env->frame_type() ==
SETTER || ....);
}

https://codereview.chromium.org/1936043002/diff/60001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1936043002/diff/60001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1593: output_frame->SetRegister(context_reg.code(), 153);
???

https://codereview.chromium.org/1936043002/diff/60001/test/mjsunit/es6/tail-c...
File test/mjsunit/es6/tail-call.js (right):

https://codereview.chromium.org/1936043002/diff/60001/test/mjsunit/es6/tail-c...
test/mjsunit/es6/tail-call.js:226: // Tail calling from getter.
Do we have tests for the non-topmost case?

[Igor Sheludko, 2016-05-06 09:50:03]

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

https://codereview.chromium.org/1936043002/diff/60001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:529: if (hydrogen_env->frame_type() == JS_FUNCTION) {
On 2016/05/03 14:42:36, Jarin wrote:
> On 2016/05/03 09:38:16, Igor Sheludko wrote:
> > Note: this is a modification to the old code.
> 
> Could you explain why the getters do not need to push here?
> 
> Would it makes sense to add an else clause that would DCHECK the expected
> frame_types; something like
> 
> } else {
>   DCHECK(hydrogen_env->frame_type() == GETTER || hydrogen_env->frame_type() ==
> SETTER || ....);
> }

Tried to rewrite a comment.

https://codereview.chromium.org/1936043002/diff/60001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1936043002/diff/60001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1593: output_frame->SetRegister(context_reg.code(), 153);
On 2016/05/03 14:42:36, Jarin wrote:
> ???

Oops.

https://codereview.chromium.org/1936043002/diff/60001/test/mjsunit/es6/tail-c...
File test/mjsunit/es6/tail-call.js (right):

https://codereview.chromium.org/1936043002/diff/60001/test/mjsunit/es6/tail-c...
test/mjsunit/es6/tail-call.js:226: // Tail calling from getter.
On 2016/05/03 14:42:36, Jarin wrote:
> Do we have tests for the non-topmost case?

Yes have normal tests (grep test/ for --inline-accessors and --inline-construct)
and some number of regression tests.

[Igor Sheludko, 2016-05-06 09:50:08]

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

[commit-bot: I haz the power, 2016-05-06 09:50:16]

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

[commit-bot: I haz the power, 2016-05-06 10:17:08]

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

[commit-bot: I haz the power, 2016-05-06 10:17:09]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-05-06 10:40:17]

lgtm with non-blocking suggestions/comments.

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

https://codereview.chromium.org/1936043002/diff/80001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:535: // So we push return value on top of outer
environment.
Could you also mention that pushing magic for the other guys (construct, getter)
is done by the deoptimizer?

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1498: bool is_topmost = (output_count_ - 1 == frame_index);
As far as I understand, is_topmost is set only if this deopt is a lazy deopt
where the top of the stack was tail-call-optimized away. If you agree, could you
possibly add a comment to that effect and insert a CHECK that this is a lazy
deopt.

Edit: Actually, now I see that you have this comment at the end of this method -
could you bring it here? (Perhaps with the CHECK?)

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1633:
output_frame->SetState(Smi::FromInt(FullCodeGenerator::NO_REGISTERS));
This should not matter, no?

[Jarin, 2016-05-06 10:41:00]

And thanks for the explanations! (Both in the code and in person.)

[Igor Sheludko, 2016-05-06 11:56:06]

Thanks!

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

https://codereview.chromium.org/1936043002/diff/80001/src/crankshaft/lithium....
src/crankshaft/lithium.cc:535: // So we push return value on top of outer
environment.
On 2016/05/06 10:40:17, Jarin wrote:
> Could you also mention that pushing magic for the other guys (construct,
getter)
> is done by the deoptimizer?

Done.

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1498: bool is_topmost = (output_count_ - 1 == frame_index);
On 2016/05/06 10:40:17, Jarin wrote:
> As far as I understand, is_topmost is set only if this deopt is a lazy deopt
> where the top of the stack was tail-call-optimized away. If you agree, could
you
> possibly add a comment to that effect and insert a CHECK that this is a lazy
> deopt.
> 
> Edit: Actually, now I see that you have this comment at the end of this method
-
> could you bring it here? (Perhaps with the CHECK?)

Done.

https://codereview.chromium.org/1936043002/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:1633:
output_frame->SetState(Smi::FromInt(FullCodeGenerator::NO_REGISTERS));
On 2016/05/06 10:40:17, Jarin wrote:
> This should not matter, no?

Indeed. Removed.

[Igor Sheludko, 2016-05-06 12:02:24]

Patchset #6 (id:120001) has been deleted

[Igor Sheludko, 2016-05-06 12:05:55]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2016-05-06 12:05:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1936043002/#ps140001
(title: "Removed too much, fixing")

[commit-bot: I haz the power, 2016-05-06 12:06:06]

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

[commit-bot: I haz the power, 2016-05-06 12:36:29]

Description was changed from

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

to

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

[commit-bot: I haz the power, 2016-05-06 12:36:31]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-05-06 12:37:26]

Description was changed from

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

to

==========
[es6] Properly handle the case when an inlined getter/setter/constructor does a
tail call.

Deoptimizer is now able to reconstruct topmost accessor and constructor frames.

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

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

[commit-bot: I haz the power, 2016-05-06 12:37:28]

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

[Benedikt Meurer, 2016-05-06 17:37:43]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-05-06 17:37:44]

LGTM.