Skip to content

relooper: Avoid recomputing strict_reachable_from in simple case #1827

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

Open
randomPoison wants to merge 2 commits into
base: master
Choose a base branch
from
Open

relooper: Avoid recomputing strict_reachable_from in simple case #1827

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6778d83
Avoid recomputing strict_reachable_from in simple case
randomPoison May 27, 2026
b052c60
Update comment wording
randomPoison Jun 22, 2026
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
18 changes: 14 additions & 4 deletions c2rust-transpile/src/cfg/relooper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ impl RelooperState {
return;
}

let local_successors = blocks
let local_successors: AdjacencyList = blocks
.iter()
.map(|(lbl, bb)| (lbl.clone(), bb.successors()))
.collect();
Expand All @@ -241,13 +241,21 @@ impl RelooperState {
// blocks. Global reachability won't work here because when we're inside a loop
// body we don't want to consider back edges, which we strip before processing
// the loop body.
let strict_reachable_from = flip_edges(transitive_closure(&local_successors));
//
// TODO: If possible we should avoid recomputing this every time `relooper` recurses. I think the
// reachability information only meaningfully changes when we strip a back edge,
// so in theory we should only need to recompute this after processing a loop.
let strict_reachable_from = || flip_edges(transitive_closure(&local_successors));

// Handle our simple case of only 1 entry. If there's no back edge to the entry
// we generate a `Simple` structure, otherwise we make a loop.
if entries.len() == 1 {
let entry = entries.first().unwrap();
if !strict_reachable_from.contains_key(entry) {
let has_back_edge = local_successors
.values()
.any(|successors| successors.contains(entry));

Comment on lines +254 to +257

@fw-immunant fw-immunant May 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

has_back_edge here seems to match the original condition here--"if the entry is in the transitive closure of any of its successors, there is a back edge". But the new condition doesn't search transitively. What justifies only looking at immediate successors? Is this connected to the comment on strict_reachable_from that says we only consider the local set of blocks? How does this relate to the comment that says "we don't want to consider back edges"--are we possibly "inside a loop" here?

@randomPoison randomPoison Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need the full transitive reachability here, we just need to know if there's a back edge to our entry in our current set of blocks since that determines if we can create a Simple or if we need to make a Loop. We can determine that from local successors alone, i.e. if one of our current blocks has the entry as its immediate successor. I used the transitive reachability info originally because it seemed convenient and we needed to compute transitive reachability anyway, but I was not taking performance into account >__>

Is this connected to the comment on strict_reachable_from that says we only consider the local set of blocks? How does this relate to the comment that says "we don't want to consider back edges"--are we possibly "inside a loop" here?

So that comment is calling out why we can't just compute reachability for the entire graph up front and then reuse that info for the entire relooping. The problem with that is that when we process a loop we strip back edges, which then changes reachability info (which is important for correctly processing the sequence of blocks within the loop body). The new comment is calling out that the reachability info only meaningfully changes when we strip back edges, so in theory we only need to recompute reachability after processing a loop body. Avoiding that recomputation might help performance when dealing with very large CFGs, but would also require some more careful thought about if stale reachability would cause problems later in relooper, so doesn't seem important to do right now.

if !has_back_edge {
let bb = blocks
.swap_remove(entry)
.expect("Entry not present in current blocks");
Expand Down Expand Up @@ -295,11 +303,13 @@ impl RelooperState {

self.relooper(new_entries, blocks, result);
} else {
self.make_loop(&strict_reachable_from, blocks, entries, result);
self.make_loop(&strict_reachable_from(), blocks, entries, result);
}
return;
}

let strict_reachable_from = strict_reachable_from();

// Sanity check that we have entries that are in our current set of blocks. We
// may have entries that aren't present in our current blocks when we're inside
// the branch of a `Multiple`, but there must always be at least one present
Expand Down
Loading