Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loop analysis simplification #9613

Merged
merged 7 commits into from
Nov 19, 2024
Merged

loop analysis simplification #9613

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
31c0af9
loop analysis simplification
KGrewal1 Nov 15, 2024
23c489a
add iterator over reverse post order
KGrewal1 Nov 15, 2024
21a55a0
review comments
KGrewal1 Nov 15, 2024
c57fb29
review comments
KGrewal1 Nov 15, 2024
eb5a7fe
use rpo iterator where possible
KGrewal1 Nov 15, 2024
0f9e83c
make use of same struct access pattern throughout
KGrewal1 Nov 15, 2024
193e7fa
use new directly instead of calling macro
KGrewal1 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 32 additions & 28 deletions cranelift/codegen/src/loop_analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,21 +198,22 @@ impl LoopAnalysis {
domtree: &DominatorTree,
layout: &Layout,
) {
// We traverse the CFG in reverse postorder
for &block in domtree.cfg_postorder().iter().rev() {
for BlockPredecessor {
inst: pred_inst, ..
} in cfg.pred_iter(block)
{
// If the block dominates one of its predecessors it is a back edge
if domtree.dominates(block, pred_inst, layout) {
// This block is a loop header, so we create its associated loop
let lp = self.loops.push(LoopData::new(block, None));
self.block_loop_map[block] = lp.into();
break;
// We break because we only need one back edge to identify a loop header.
}
}
for &block in domtree
.cfg_postorder()
.into_iter()
.rev() // We traverse the CFG in reverse postorder
.filter(|&&block| {
// If the block dominates any one of its predecessors it is a back edge
cfg.pred_iter(block).any(
|BlockPredecessor {
inst: pred_inst, ..
}| domtree.dominates(block, pred_inst, layout),
)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This iterator chain is a little too convoluted to be easily readable, IMHO. One thing that might help is to pull out an is_loop_header helper?

It might also be nice to have a method on the domtree that returns an RPO iterator directly (impl Iterator... in return is fine). Then we could do something like:

for &block in domtree.cfg_rpo().filter(|&block| is_loop_header(cfg, domtree, block)) { ... }

{
// This block is a loop header, so we create its associated loop
let lp = self.loops.push(LoopData::new(block, None));
self.block_loop_map[block] = lp.into();
}
}

Expand All @@ -229,16 +230,18 @@ impl LoopAnalysis {
// We handle each loop header in reverse order, corresponding to a pseudo postorder
// traversal of the graph.
for lp in self.loops().rev() {
for BlockPredecessor {
block: pred,
inst: pred_inst,
} in cfg.pred_iter(self.loops[lp].header)
{
// We follow the back edges
if domtree.dominates(self.loops[lp].header, pred_inst, layout) {
stack.push(pred);
}
}
stack.extend(
cfg.pred_iter(self.loops[lp].header)
.filter(
|BlockPredecessor {
inst: pred_inst, ..
}| {
// We follow the back edges
domtree.dominates(self.loops[lp].header, *pred_inst, layout)
},
)
.map(|BlockPredecessor { block, .. }| block),
);
KGrewal1 marked this conversation as resolved.
Show resolved Hide resolved
while let Some(node) = stack.pop() {
let continue_dfs: Option<Block>;
match self.block_loop_map[node].expand() {
Expand Down Expand Up @@ -283,9 +286,10 @@ impl LoopAnalysis {
// Now we have handled the popped node and need to continue the DFS by adding the
// predecessors of that node
if let Some(continue_dfs) = continue_dfs {
for BlockPredecessor { block: pred, .. } in cfg.pred_iter(continue_dfs) {
stack.push(pred)
}
stack.extend(
cfg.pred_iter(continue_dfs)
.map(|BlockPredecessor { block: pred, .. }| pred),
);
}
}
}
Expand Down