[turboshaft][csa] Support block positioning builtin pgo [v8/v8 : main]
Chen, Yolanda (Gerrit)
Chen, Yolanda voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chen, Yolanda
| Commit-Queue | +1 |
1 comment
Hi Darius, Nico and Leszek,
The design doc is at https://docs.google.com/document/d/1d2nbX9iNPQM9R2GjJOoeUjjOq--d3mjnp807FgsMlkU/edit?pli=1&tab=t.0#heading=h.2y7d90lyfo9f
PTAL, Thanks!
Related details
- Darius Mercadier
- Leszek Swirski
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann added 8 comments![]( "Open in Gerrit")
namespace v8 {
namespace internal {
namespace compiler {nit: `namespace v8::internal::compiler {`
namespace v8 {
namespace internal {
namespace compiler {nit: `namespace v8::internal::compiler {`
std::set<RpoNumber> chain;nit: Maybe rename this to `visited` as well (or `placed_blocks` or something). `Chain` gives a different intuition of what this tracks (to me).
InstructionBlocks* block_permutation =
zone()->AllocateArray<InstructionBlocks>(1);
new (block_permutation) InstructionBlocks(
static_cast<int>(permutation.size()), nullptr, zone());Is this required? Why not just `zone()->New<InstructionBlocks>(...)`?
void ReorderBlocks(base::Vector<int32_t> permutation) {I feel some of the `CHECK`s in this functions can be turned into `DCHECK`s. You can also leave a TODO to switch this after we get some coverage.
#ifdef BUILTIN_BLOCK_POSITIONI would prefer to not duplicate those fields and instead make them generally non-const or is there any disadvantage with that?
(same in a few other places and files)
if (max_count < min_count * 40) {A comment would be good here. Also if you have some insight on why `40` is the number to go here, mention it. If it's just an arbitrary pick, note that as well. This will help us in the future to decide if we can tweak this number or if there are measurements that back `40` as the best choice.
RUN_MAYBE_ABORT(BlockPositioningPhase);By running this after register allocation, will this not change life ranges and break register assignment?
Related details
- Chen, Yolanda
- Darius Mercadier
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda added 9 comments![]( "Open in Gerrit")
Thanks for the comments. I've uploaded a new patchset to resolve the comments and fixed some debug check failures when enable the profile.
nit: `namespace v8::internal::compiler {`
Done
nit: `namespace v8::internal::compiler {`
Done
nit: Maybe rename this to `visited` as well (or `placed_blocks` or something). `Chain` gives a different intuition of what this tracks (to me).
Done
InstructionBlocks* block_permutation =
zone()->AllocateArray<InstructionBlocks>(1);
new (block_permutation) InstructionBlocks(
static_cast<int>(permutation.size()), nullptr, zone());Is this required? Why not just `zone()->New<InstructionBlocks>(...)`?
I was following InstructionBlocksFor. I tried your suggestion. It also works!
void ReorderBlocks(base::Vector<int32_t> permutation) {I feel some of the `CHECK`s in this functions can be turned into `DCHECK`s. You can also leave a TODO to switch this after we get some coverage.
Done. Do you think we need to create an issue to track this?
I would prefer to not duplicate those fields and instead make them generally non-const or is there any disadvantage with that?
(same in a few other places and files)
I was unsure if any unexpected changes to the default pipeline. I tried to build and test with non-const by default, no failures.
A comment would be good here. Also if you have some insight on why `40` is the number to go here, mention it. If it's just an arbitrary pick, note that as well. This will help us in the future to decide if we can tweak this number or if there are measurements that back `40` as the best choice.
Done
By running this after register allocation, will this not change life ranges and break register assignment?
This only changes code position, but not the execution order. All registers are assigned and used as before in the original block. The jumps and targets are also unchanged. The live range is only valid after SpecialRPO where back-edges point to loop headers only, but not other blocks. After the new placement, it does not quaranteer linear live range anymore. That's why we need to put the positioning phase after register-allocation.
Related details
- Darius Mercadier
- Leszek Swirski
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Hartmann
| Code-Review | +1 |
1 comment
- Chen, Yolanda
- Darius Mercadier
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georgia Kouveli (Gerrit)
Georgia Kouveli added 2 comments![]( "Open in Gerrit")
I hope you don't mind a question - I happened to be looking at something related and stumbled upon your CL. :)
cur_block->set_loop_end(Perhaps a silly question, but does the reordering algorithm guarantee that loop bodies would be contiguous and loop_end still makes sense? It's not obvious that it does.
The loop rotation code in ComputeAssemblyOrder seems to rely on this.
Related details
- Chen, Yolanda
- Darius Mercadier
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda added 2 comments![]( "Open in Gerrit")
Hi Nico, I've made some correction on the loop_end update to support the later loop rotation. Please take a look. Thanks!
Perhaps a silly question, but does the reordering algorithm guarantee that loop bodies would be contiguous and loop_end still makes sense? It's not obvious that it does.
The loop rotation code in ComputeAssemblyOrder seems to rely on this.
Good question! Yes, some of the cold blocks may be moved out of the loop body, this is similar to the deferred blocks. But we still can guranteer contiguous of loop body for the hot blocks using the top-down positioning. Thus loop rotation can be performed after the reordering (ideally we may combine them, but currently we only apply this for builtin pgo). To ensure this, I've updated the code to add some check and update the loop_end to point to one plus the backedge block number instead of direct mapping to the old number.
Related details
- Darius Mercadier
- Georgia Kouveli
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: BUILD.gn
Insertions: 8, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/compiler/turboshaft/instruction-selection-phase.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/compiler/backend/instruction.h
Insertions: 7, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
[turboshaft][csa] Support block positioning builtin pgo
- M BUILD.gn
- M src/builtins/profile-data-reader.cc
- M src/builtins/profile-data-reader.h
- A src/compiler/backend/block-position.cc
- A src/compiler/backend/block-position.h
- M src/compiler/backend/instruction.cc
- M src/compiler/backend/instruction.h
- M src/compiler/turboshaft/graph.cc
- M src/compiler/turboshaft/graph.h
- M src/compiler/turboshaft/instruction-selection-phase.cc
- M src/compiler/turboshaft/pipelines.h
- M src/compiler/turboshaft/register-allocation-phase.h
- M src/logging/runtime-call-stats.h
- M tools/builtins-pgo/get_hints.py
Code-Review: +1 by Nico Hartmann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Michael Achenbach has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |