Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505933008:
[compiler] Ensure code unsupported by Crankshaft goes to Ignition. (Closed)
Description[compiler] Ensure code unsupported by Crankshaft goes to Ignition.
BUG=v8:4280, v8:5657
Committed: https://crrev.com/5f5300a61bfe51bb1ef9b5ca8709db38ce81b37d
Cr-Commit-Position: refs/heads/master@{#41209}
Patch Set 1 #Patch Set 2 : Update FunctionTester #
Total comments: 1
Patch Set 3 : Update name to must_use_ignition_turbo #Patch Set 4 : Rebase #Patch Set 5 : Fix issues with --always-opt and FunctionTester #Patch Set 6 : cctests now pass #Patch Set 7 : Fix always-opt and asm->wasm #Patch Set 8 : Disable two failing tests #
Total comments: 8
Patch Set 9 : Rebase and fix inlining issue with ShouldUseIgnition code. #Patch Set 10 : Address comments #Patch Set 11 : Fix GetBaselineCode #
Dependent Patchsets: Messages
Total messages: 75 (58 generated)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
Michi - this ensures that anything that can't be Crankshafted will go through the new pipeline. I think we need a new name for dont_crankshaft, but I can't decide on a good one. Suggestions: - dont_use_crankshaft_pipeline - unsupported_by_deprecated_pipeline - use_new_compile_pipeline - use_ignition Any better ideas?
ccing Peter since Michi mentioned you are wanting to deprecate super support from FCG. Once (if) this CL lands we can remove any code for parts which the ast-numbering phase says dont_crankshaft.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am fine with this approach. Please wait with landing this until my removal for FCG-generator support has landed and then rebase onto it. Thanks! This is the CL I am referring to: https://codereview.chromium.org/2504223002/
On 2016/11/18 11:43:27, rmcilroy wrote: > Michi - this ensures that anything that can't be Crankshafted will go through > the new pipeline. I think we need a new name for dont_crankshaft, but I can't > decide on a good one. Suggestions: > - dont_use_crankshaft_pipeline > - unsupported_by_deprecated_pipeline > - use_new_compile_pipeline > - use_ignition > > Any better ideas? I don't have a strong opinion about the name either. Here are just a bunch of random ideas (in order of preference) to add to the pool: - use_bytecode - use_ignition - must_use_bytecode - must_use_ignition
On 2016/11/18 12:12:24, Michael Starzinger wrote: > On 2016/11/18 11:43:27, rmcilroy wrote: > > Michi - this ensures that anything that can't be Crankshafted will go through > > the new pipeline. I think we need a new name for dont_crankshaft, but I can't > > decide on a good one. Suggestions: > > - dont_use_crankshaft_pipeline > > - unsupported_by_deprecated_pipeline > > - use_new_compile_pipeline > > - use_ignition > > > > Any better ideas? > > I don't have a strong opinion about the name either. Here are just a bunch of > random ideas (in order of preference) to add to the pool: > - use_bytecode > - use_ignition > - must_use_bytecode > - must_use_ignition These are nice and simple, but might get confusing since we might use bytecode on functions without this bit set (e.g., ignition-staging) and those still need to be able to tier up via fullcodegen. How about must_use_ignition_turbo ? https://codereview.chromium.org/2505933008/diff/20001/test/cctest/compiler/fu... File test/cctest/compiler/function-tester.cc (right): https://codereview.chromium.org/2505933008/diff/20001/test/cctest/compiler/fu... test/cctest/compiler/function-tester.cc:170: info.MarkAsOptimizeFromBytecode(); I updated this as well to ensure we always go through the BytecodeGraphBuilder for FunctionTester (since some bits being tested, e.g., Eval, will be ripped out of FCG / the ASTGraphBuilder soon).
Description was changed from ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280 ========== to ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280,v8:5657 ==========
Rebased to include generator CL and renamed flag. Michi PTAL
LGTM. Thanks!
The CQ bit was checked by rmcilroy@chromium.org
The CQ bit was unchecked by rmcilroy@chromium.org
rmcilroy@chromium.org changed reviewers: + bmeurer@chromium.org
bmeurer@chromium.org: Please review changes in crankshaft/ Thanks.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29001)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.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 master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18229) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Michi: some fixes were required to FunctionTester and for always-opt. Please take a look at the changes in the last patch set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.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 master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.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 master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
On 2016/11/21 16:57:35, rmcilroy wrote: > Michi: some fixes were required to FunctionTester and for always-opt. Please > take a look at the changes in the last patch set. Actually there are still issues with this on some configurations, I'll have a look tomorrow and get back to you.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18264) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
LGTM, thanks.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Patchset #8 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated to fix tests. The issue was that on --always-opt, we hadn't always parsed the code before we checked ShouldUseIgnition and so tried to compile code with FCG/Crankshaft which would be must_use_ignition_turbo after the renumbering phase. I've fixed this by making sure that code is compiled before we ever try to optimize it (this should only affect test code with always-opt / OptimizeFunctionOnNextCall, since real code should always be compiled before trying to optimize). Michi, can you take another look please. https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc#newcod... src/compiler.cc:1710: function->shared()->is_compiled()) { With this change we only mark for optimization if the code is already compiled. This shouldn't change anything AFAIK though, since if it's not compiled it will run GetLazyCode which will optimize the code if FLAG_always_opt is on anway. https://codereview.chromium.org/2505933008/diff/180001/test/debugger/debugger... File test/debugger/debugger.status (right): https://codereview.chromium.org/2505933008/diff/180001/test/debugger/debugger... test/debugger/debugger.status:34: 'debug/debug-evaluate-locals': [FAIL], This fails in the same way as turbo_opt below. It fails now because a couple of the functions have eval / with statements, which means they now get optimized by TurboFan
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12984) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
LGTM on the latest iteration as well. A couple of new comments though. https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc#newcod... src/compiler.cc:1710: function->shared()->is_compiled()) { On 2016/11/22 14:03:44, rmcilroy wrote: > With this change we only mark for optimization if the code is already compiled. > This shouldn't change anything AFAIK though, since if it's not compiled it will > run GetLazyCode which will optimize the code if FLAG_always_opt is on anway. Acknowledged. I thought about this for a little bit and I also don't see any particular pattern where we would loose too much test coverage. I think the "optimized code map" should ensure we are preferring any pre-existing optimized code over the shared unoptimized code in {GetLazyCode} and the corresponding builtin. https://codereview.chromium.org/2505933008/diff/180001/src/full-codegen/full-... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/full-codegen/full-... src/full-codegen/full-codegen.cc:87: if (info->shared_info()->must_use_ignition_turbo()) return false; I assume turning this into a DCHECK doesn't work yet? Can we leave a TODO here that this should eventually become a DCHECK? https://codereview.chromium.org/2505933008/diff/180001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/runtime/runtime-te... src/runtime/runtime-test.cc:131: if (!function->is_compiled() && Would it be possible to check whether {function->shared()->is_compiled()} here instead? Note that {Compiler::Compile} already has the appropriate post-condition check for {function->shared()->is_compiled()} in place. WDYT? Also I would move this to after the quick-check for {function->IsOptimized()} below (i.e. 3 lines down).
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/compiler.cc#newcod... src/compiler.cc:1710: function->shared()->is_compiled()) { On 2016/11/22 14:53:50, Michael Starzinger wrote: > On 2016/11/22 14:03:44, rmcilroy wrote: > > With this change we only mark for optimization if the code is already > compiled. > > This shouldn't change anything AFAIK though, since if it's not compiled it > will > > run GetLazyCode which will optimize the code if FLAG_always_opt is on anway. > > Acknowledged. I thought about this for a little bit and I also don't see any > particular pattern where we would loose too much test coverage. I think the > "optimized code map" should ensure we are preferring any pre-existing optimized > code over the shared unoptimized code in {GetLazyCode} and the corresponding > builtin. Acknowledged. https://codereview.chromium.org/2505933008/diff/180001/src/full-codegen/full-... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/full-codegen/full-... src/full-codegen/full-codegen.cc:87: if (info->shared_info()->must_use_ignition_turbo()) return false; On 2016/11/22 14:53:50, Michael Starzinger wrote: > I assume turning this into a DCHECK doesn't work yet? Can we leave a TODO here > that this should eventually become a DCHECK? The issue was EnsureDeoptimizationSupport. I've moved the check to EnsureDeoptimizationSupport and made this a DCHECK here. https://codereview.chromium.org/2505933008/diff/180001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2505933008/diff/180001/src/runtime/runtime-te... src/runtime/runtime-test.cc:131: if (!function->is_compiled() && On 2016/11/22 14:53:50, Michael Starzinger wrote: > Would it be possible to check whether {function->shared()->is_compiled()} here > instead? Note that {Compiler::Compile} already has the appropriate > post-condition check for {function->shared()->is_compiled()} in place. WDYT? Done. > Also I would move this to after the quick-check for {function->IsOptimized()} > below (i.e. 3 lines down). This is what I had originally but it doesn't work because when --always-opt is enabled the Compiler::Compile causes the code to get optimized, and MarkForOptimization fails due to the code already being optimized.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.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 master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16665) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2505933008/#ps240001 (title: "Fix GetBaselineCode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1479891702948500,
"parent_rev": "9237dff5012759c1f6e959ffe04ba4e1a9b80a4f", "commit_rev":
"9f79c54b1e54194d6543b7d4aa6970203b0ecdbd"}
Message was sent while issue was closed.
Description was changed from ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280,v8:5657 ========== to ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280,v8:5657 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280,v8:5657 ========== to ========== [compiler] Ensure code unsupported by Crankshaft goes to Ignition. BUG=v8:4280,v8:5657 Committed: https://crrev.com/5f5300a61bfe51bb1ef9b5ca8709db38ce81b37d Cr-Commit-Position: refs/heads/master@{#41209} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5f5300a61bfe51bb1ef9b5ca8709db38ce81b37d Cr-Commit-Position: refs/heads/master@{#41209} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
