Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262033003:
[turbofan] Allow inlining into BytecodeGraphBuilder graph. (Closed)
|
Created:
4 years, 4 months ago by Michael Starzinger Modified:
4 years, 3 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@local_interpreter-preserve-bytecode-5 Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
Committed: https://crrev.com/a400590761c6afce83373935f667eb3fc4029ca1
Cr-Commit-Position: refs/heads/master@{#39439}
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Rebased. #
Total comments: 4
Patch Set 4 : Rebased. #Patch Set 5 : Rebased. #Patch Set 6 : Rebased. #
Messages
Total messages: 33 (25 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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11343) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11423) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
Description was changed from
==========
[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=
TEST=cctest/test-run-inlining
BUG=v8:5251
==========
to
==========
[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
==========
mstarzinger@chromium.org changed reviewers: + bmeurer@chromium.org, rmcilroy@chromium.org
Nice! LGTM. https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... File src/compiler/js-inlining.cc (right): https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... src/compiler/js-inlining.cc:414: Do we want todos about the loop analysis and Tyler phases, or are they never going to be used on bytecode graph builder graphs?
https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... File src/compiler/js-inlining.cc (right): https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... src/compiler/js-inlining.cc:414: On 2016/08/25 08:53:26, rmcilroy wrote: > Do we want todos about the loop analysis and Tyler phases, or are they never > going to be used on bytecode graph builder graphs? The LoopAssignmentAnalyzer is an AST-based analysis. This is also excluded in pipeline.cc for the same reason. Same goes for the TypeHintAnalyzer, which is based on code object from full-codegen and not needed if all feedback goes via the type-feedback vector. Not sure TODOs here help in any way. There are no TODOs in pipeline.cc either.
https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... File src/compiler/js-inlining.cc (right): https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... src/compiler/js-inlining.cc:414: On 2016/08/25 09:01:26, Michael Starzinger wrote: > On 2016/08/25 08:53:26, rmcilroy wrote: > > Do we want todos about the loop analysis and Tyler phases, or are they never > > going to be used on bytecode graph builder graphs? > > The LoopAssignmentAnalyzer is an AST-based analysis. This is also excluded in > pipeline.cc for the same reason. Same goes for the TypeHintAnalyzer, which is > based on code object from full-codegen and not needed if all feedback goes via > the type-feedback vector. > > Not sure TODOs here help in any way. There are no TODOs in pipeline.cc either. Makes sense, thanks for the explanation. https://codereview.chromium.org/2262033003/diff/40001/src/compiler/js-inlinin... src/compiler/js-inlining.cc:414: On 2016/08/25 09:01:26, Michael Starzinger wrote: > On 2016/08/25 08:53:26, rmcilroy wrote: > > Do we want todos about the loop analysis and Tyler phases, or are they never > > going to be used on bytecode graph builder graphs? > > The LoopAssignmentAnalyzer is an AST-based analysis. This is also excluded in > pipeline.cc for the same reason. Same goes for the TypeHintAnalyzer, which is > based on code object from full-codegen and not needed if all feedback goes via > the type-feedback vector. > > Not sure TODOs here help in any way. There are no TODOs in pipeline.cc either. Makes sense, thanks for the explanation.
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...
Patchset #4 (id:60001) has been deleted
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: This issue passed the CQ dry run.
Nice. 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...
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 mstarzinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2262033003/#ps120001 (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] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
==========
to
==========
[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
==========
to
==========
[turbofan] Allow inlining into BytecodeGraphBuilder graph.
This is a first implementation of inlining into graphs that have been
created using the {BytecodeGraphBuilder}. Note that inlining sticks to
graphs of the same kind, we only ever inline AstGraph into AstGraph or
BytecodeGraph into BytecodeGraph, no mixed inlining.
R=bmeurer@chromium.org,rmcilroy@chromium.org
TEST=cctest/test-run-inlining
BUG=v8:5251
Committed: https://crrev.com/a400590761c6afce83373935f667eb3fc4029ca1
Cr-Commit-Position: refs/heads/master@{#39439}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a400590761c6afce83373935f667eb3fc4029ca1 Cr-Commit-Position: refs/heads/master@{#39439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
