(feat): dominator analysis using forward dataflow#9871
(feat): dominator analysis using forward dataflow#9871eytan-starkware wants to merge 1 commit intomainfrom
Conversation
|
This change is |
PR SummaryLow Risk Overview Adds unit and file-based tests (new Reviewed by Cursor Bugbot for commit d3a2d1c. Bugbot is set up for automated code reviews on this repo. Configure here. |
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 2 comments.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-lowering/src/analysis/dom.rs line 0 at r1 (raw file):
dominator - use full name.
crates/cairo-lang-lowering/src/analysis/dom.rs line 16 at r1 (raw file):
pub struct Dominators { /// dom_sets for a block_id = set of blocks that dominate block_id. dom_sets: Vec<Option<OrderedHashSet<BlockId>>>,
why Option?
Code quote:
dom_sets: Vec<Option<OrderedHashSet<BlockId>>>,
eytan-starkware
changed the base branch from
eytan_graphite/_feat_def_site_analysis_based_on_forward_dataflow
to
graphite-base/9871
eytan-starkware
force-pushed
the
graphite-base/9871
branch
from
21bf47f to
e4d7e3f
Compare
eytan-starkware
force-pushed
the
eytan_graphite/_feat_dominator_analysis_using_forward_dataflow
branch
from
89d473e to
d3a2d1c
Compare
eytan-starkware
left a comment
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware made 2 comments.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on orizi and TomerStarkware).
crates/cairo-lang-lowering/src/analysis/dom.rs line 16 at r1 (raw file):
Previously, orizi wrote…
why Option?
No good enough reason. Changed.
crates/cairo-lang-lowering/src/analysis/dom.rs line at r1 (raw file):
Previously, orizi wrote…
dominator - use full name.
Done.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d3a2d1c. Configure here.
| /// Runs dominator analysis on a lowered function. | ||
| pub fn analyze(lowered: &Lowered<'_>) -> Self { | ||
| let mut fwd = ForwardDataflowAnalysis::new(lowered, DominatorAnalyzer); | ||
| let per_block = fwd.run().into_iter().map(|opt| opt.unwrap_or_default()).collect(); |
There was a problem hiding this comment.
Unreachable blocks get empty set, not {root}
Medium Severity
The doc comment on per_block states unreachable blocks default to {root}, but unwrap_or_default() produces an empty OrderedHashSet. This means dominates(BlockId::root(), unreachable_block) returns false, violating both the documented contract and the standard dominator invariant that the entry block dominates all blocks. Downstream analyses relying on this invariant will behave incorrectly for unreachable blocks.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d3a2d1c. Configure here.
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi partially reviewed 5 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-lowering/src/analysis/dominator.rs line 70 at r2 (raw file):
} #[cfg(test)]
move to test file.
crates/cairo-lang-lowering/src/analysis/dominator_test.rs line 46 at r2 (raw file):
dom_list.sort(); let dom_strs: Vec<String> = dom_list.iter().map(|d| format!("blk{d}")).collect(); format!("blk{}: {{{}}}", block_id.0, dom_strs.join(", "))
Suggestion:
let doms = dominators.dominators_of(block_id);
let dom_strs = doms.iter().map(|d| d.0).sorted().map(|d| format!("blk{d}");
format!("blk{}: {{{}}}", block_id.0, dom_strs.join(", "))crates/cairo-lang-lowering/src/analysis/dominator_test.rs line 48 at r2 (raw file):
format!("blk{}: {{{}}}", block_id.0, dom_strs.join(", ")) }) .collect::<Vec<_>>()
3 participants
Summary
Adds a block dominator analysis pass to the lowering IR. The analysis computes, for each block, the set of all blocks that dominate it (i.e., every path from the entry block to that block must pass through each dominator). The implementation uses the existing forward dataflow framework, with set intersection as the merge operation and self-insertion as the transfer function.
A
Dominatorsstruct is exposed withdominates(a, b)anddominators_of(block)query methods. File-based tests cover linear functions, if-else without a merge block, diamond CFGs, nested conditionals, and multi-arm matches.Type of change
Please check one:
Why is this change needed?
Dominator information is a foundational building block for many compiler analyses and optimizations (e.g., loop detection, SSA construction, code motion). Without a dedicated dominator pass, downstream analyses must either recompute dominance ad hoc or forgo dominator-dependent optimizations entirely.
What was the behavior or documentation before?
No dominator analysis existed in the lowering analysis crate.
What is the behavior or documentation after?
Callers can run
Dominators::analyze(&lowered)on a lowered function body and query whether one block dominates another, or retrieve the full dominator set for any block.Related issue or discussion (if any)
N/A
Additional context
The implementation piggybacks on the existing
ForwardDataflowAnalysisinfrastructure. The merge step performs set intersection across predecessor dominator sets, and the transfer step adds the current block to its own dominator set, correctly encoding the standard dominator lattice.