Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709583002:
[turbofan] Fixing ES6 tail calls in Turbofan. (Closed)
|
Created:
4 years, 10 months ago by Igor Sheludko Modified:
4 years, 10 months ago Reviewers:
Jarin CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Fixing ES6 tail calls in Turbofan.
In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site.
Otherwise we will see G in a stack trace inside H.
This CL also adds a "megatest" which tests product of the following cases:
1) tail caller is inlined/not-inlined
2) tail callee is inlined/not-inlined
3) tail caller has an arguments adaptor frame above or not
4) tail callee has an arguments adaptor frame above or not
5) tail callee is a normal/bound/proxy function
Note that tests for not yet supported cases are not run for now.
BUG=v8:4698
LOG=N
Committed: https://crrev.com/c67b5096cd81af4bdf62591f36e5eb72a1c7446a
Cr-Commit-Position: refs/heads/master@{#34108}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressing comments #
Total comments: 2
Patch Set 3 : Addressing comments #
Dependent Patchsets: Messages
Total messages: 30 (16 generated)
Description was changed from ========== [es6] [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. BUG=v8:4698 LOG=N ========== to ========== [es6] [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a tail call megatest generator (test/mjsunit/es6/tail-call-megatest-generator.js) which generates all possible cases for the product of the following dimensions: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
Description was changed from ========== [es6] [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a tail call megatest generator (test/mjsunit/es6/tail-call-megatest-generator.js) which generates all possible cases for the product of the following dimensions: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [es6] [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709583002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709583002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [es6] [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G that tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
ishell@chromium.org changed reviewers: + jarin@chromium.org
PTAL
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ==========
Looking good, but the megatests are a bit confusing. https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... File test/mjsunit/es6/tail-call-megatest-generator.js (right): https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... test/mjsunit/es6/tail-call-megatest-generator.js:194: generate_test(); How about running the generated test through eval rather than outputting a file? I have seen this done elsewhere.
https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... File test/mjsunit/es6/tail-call-megatest-generator.js (right): https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... test/mjsunit/es6/tail-call-megatest-generator.js:194: generate_test(); On 2016/02/18 07:51:41, Jarin wrote: > How about running the generated test through eval rather than outputting a file? > > I have seen this done elsewhere. I did that initially, but when something fails in eval we don't have positions anymore and it's harder to figure out what went wrong than in non-eval case.
On 2016/02/18 07:56:32, Igor Sheludko wrote: > https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... > File test/mjsunit/es6/tail-call-megatest-generator.js (right): > > https://codereview.chromium.org/1709583002/diff/1/test/mjsunit/es6/tail-call-... > test/mjsunit/es6/tail-call-megatest-generator.js:194: generate_test(); > On 2016/02/18 07:51:41, Jarin wrote: > > How about running the generated test through eval rather than outputting a > file? > > > > I have seen this done elsewhere. > > I did that initially, but when something fails in eval we don't have positions > anymore and it's harder to figure out what went wrong than in non-eval case. I understand that point, but once you are done with your work, the test should not really break so there should not be a need to have source position. (Even if the test fails, it is trivial to print the eval string and run it separately.)
I addressed comments. PTAL
lgtm. https://codereview.chromium.org/1709583002/diff/20001/test/mjsunit/es6/tail-c... File test/mjsunit/es6/tail-call-megatest.js (right): https://codereview.chromium.org/1709583002/diff/20001/test/mjsunit/es6/tail-c... test/mjsunit/es6/tail-call-megatest.js:38: // Don't inline. Don't inline. Don't inline. Don't inline. Could not you use %NeverOptimizeFunction to prevent inlining?
https://codereview.chromium.org/1709583002/diff/20001/test/mjsunit/es6/tail-c... File test/mjsunit/es6/tail-call-megatest.js (right): https://codereview.chromium.org/1709583002/diff/20001/test/mjsunit/es6/tail-c... test/mjsunit/es6/tail-call-megatest.js:38: // Don't inline. Don't inline. Don't inline. Don't inline. On 2016/02/18 09:08:08, Jarin wrote: > Could not you use %NeverOptimizeFunction to prevent inlining? Good point. For utility functions it makes sense, but not for f() and g() because I still want them to be optimized (to test TF).
The CQ bit was checked by ishell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709583002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709583002/40001
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "tail call megatest generator" which generates tests for a product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note, that tests for not yet supported cases are not generated for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "megatest" which tests product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note that tests for not yet supported cases are not run for now. BUG=v8:4698 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ishell@chromium.org
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/1709583002/#ps40001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709583002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709583002/40001
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "megatest" which tests product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note that tests for not yet supported cases are not run for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "megatest" which tests product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note that tests for not yet supported cases are not run for now. BUG=v8:4698 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "megatest" which tests product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note that tests for not yet supported cases are not run for now. BUG=v8:4698 LOG=N ========== to ========== [turbofan] Fixing ES6 tail calls in Turbofan. In case when F inlined normal call to G which tail calls H we should not write translation for G for the tail call site. Otherwise we will see G in a stack trace inside H. This CL also adds a "megatest" which tests product of the following cases: 1) tail caller is inlined/not-inlined 2) tail callee is inlined/not-inlined 3) tail caller has an arguments adaptor frame above or not 4) tail callee has an arguments adaptor frame above or not 5) tail callee is a normal/bound/proxy function Note that tests for not yet supported cases are not run for now. BUG=v8:4698 LOG=N Committed: https://crrev.com/c67b5096cd81af4bdf62591f36e5eb72a1c7446a Cr-Commit-Position: refs/heads/master@{#34108} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c67b5096cd81af4bdf62591f36e5eb72a1c7446a Cr-Commit-Position: refs/heads/master@{#34108} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
