Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363413005:
[turbofan] Enable BytecodeGraphBuilder by default. (Closed)
|
Created:
4 years, 2 months ago by Michael Starzinger Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
Committed: https://crrev.com/98e3ed6b215087da7d8f8c88892c70831a050197
Cr-Commit-Position: refs/heads/master@{#40045}
Patch Set 1 #Patch Set 2 : Fix function tester. #Patch Set 3 : Rebased. #
Total comments: 2
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by mstarzinger@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_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by mstarzinger@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...
Description was changed from
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=
==========
to
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
==========
mstarzinger@chromium.org changed reviewers: + bmeurer@chromium.org, hablich@chromium.org, rmcilroy@chromium.org
Michael: I would like to land this right after the 5.5 branch cut. Benedikt: PTAL. Ross: FYI.
Awesome. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice, thanks for putting the CL together. We should also be making ShouldUseIgnition always return true with UseTurboFan is true so that we always start in Ignition for these functions as well, shouldn't we? I'm fine to do that in a separate CL though (happy to put one together myself later this week).
On 2016/09/28 13:16:12, rmcilroy wrote: > Nice, thanks for putting the CL together. > > We should also be making ShouldUseIgnition always return true with UseTurboFan > is true so that we always start in Ignition for these functions as well, > shouldn't we? I'm fine to do that in a separate CL though (happy to put one > together myself later this week). Yes, but I'd like to do that in a separate CL.
On 2016/09/28 13:18:49, Michael Starzinger wrote: > On 2016/09/28 13:16:12, rmcilroy wrote: > > Nice, thanks for putting the CL together. > > > > We should also be making ShouldUseIgnition always return true with UseTurboFan > > is true so that we always start in Ignition for these functions as well, > > shouldn't we? I'm fine to do that in a separate CL though (happy to put one > > together myself later this week). > > Yes, but I'd like to do that in a separate CL. lgtm
The CQ bit was checked by mstarzinger@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/2363413005/diff/40001/test/cctest/compiler/fu... File test/cctest/compiler/function-tester.cc (right): https://codereview.chromium.org/2363413005/diff/40001/test/cctest/compiler/fu... test/cctest/compiler/function-tester.cc:173: if (Compiler::EnsureBytecode(&info)) { Just thinking about this again - this basically makes it impossible to use the ASTGraphBuilder even when turbo_from_bytecode is false, right? This means we can't do (local) experiments comparing graphs built with ASTGraphBuilder and BytecodeGraphBuilder. Should we allow that ability until we've got rid of all the regressions between BCGB and ASTGraphBuilder?
https://codereview.chromium.org/2363413005/diff/40001/test/cctest/compiler/fu... File test/cctest/compiler/function-tester.cc (right): https://codereview.chromium.org/2363413005/diff/40001/test/cctest/compiler/fu... test/cctest/compiler/function-tester.cc:173: if (Compiler::EnsureBytecode(&info)) { On 2016/10/06 14:22:57, rmcilroy wrote: > Just thinking about this again - this basically makes it impossible to use the > ASTGraphBuilder even when turbo_from_bytecode is false, right? This means we > can't do (local) experiments comparing graphs built with ASTGraphBuilder and > BytecodeGraphBuilder. Should we allow that ability until we've got rid of all > the regressions between BCGB and ASTGraphBuilder? This is true for cctest, yes. But compiler.cc still has support for --noturbo-from-bytecode for (local) experimentation via the d8 shell.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/06 14:26:12, Michael Starzinger wrote: > https://codereview.chromium.org/2363413005/diff/40001/test/cctest/compiler/fu... > File test/cctest/compiler/function-tester.cc (right): > > https://codereview.chromium.org/2363413005/diff/40001/test/cctest/compiler/fu... > test/cctest/compiler/function-tester.cc:173: if > (Compiler::EnsureBytecode(&info)) { > On 2016/10/06 14:22:57, rmcilroy wrote: > > Just thinking about this again - this basically makes it impossible to use the > > ASTGraphBuilder even when turbo_from_bytecode is false, right? This means we > > can't do (local) experiments comparing graphs built with ASTGraphBuilder and > > BytecodeGraphBuilder. Should we allow that ability until we've got rid of all > > the regressions between BCGB and ASTGraphBuilder? > > This is true for cctest, yes. But compiler.cc still has support for > --noturbo-from-bytecode for (local) experimentation via the d8 shell. Sorry ignore me, I thought the function-tester.cc changes were in compiler.cc.
The CQ bit was checked by mstarzinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, hablich@chromium.org Link to the patchset: https://codereview.chromium.org/2363413005/#ps40001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
==========
to
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
==========
to
==========
[turbofan] Enable BytecodeGraphBuilder by default.
This enables the {BytecodeGraphBuilder} whenever heuristics in the
compilation pipeline determine both Ignition and TurboFan to be used.
There no longer needs to be an explicit flag passed in order to build
graphs from bytecode.
R=bmeurer@chromium.org
Committed: https://crrev.com/98e3ed6b215087da7d8f8c88892c70831a050197
Cr-Commit-Position: refs/heads/master@{#40045}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/98e3ed6b215087da7d8f8c88892c70831a050197 Cr-Commit-Position: refs/heads/master@{#40045} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
