fix Region.clone() to use reverse-post-order traversal#635
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Region.clone() previously iterated blocks in index order, which could leave cloned operands pointing at the original region when aggressive fold produced out-of-definition-order block layouts. Now clones statements in reverse post-order of the CFG, and raises ValueError if an in-region def is used before being cloned. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Roger-luo
left a comment
Roger-luo
left a comment
There was a problem hiding this comment.
I think this is a good change to canonicalize the order of iteration, but I believe it should not be part of the IR iterator but should be a rewrite pass - otherwise we need to pay the price everytime we try to iterate the blocks
Move the RPO traversal out of Region.clone() into a dedicated SortBlocks rewrite rule that mutates the block sequence in-place. Integrate it into CompactifyRegion so blocks are sorted after CFG compactification. Region.clone() now iterates self.blocks (which are already in RPO order) with a safety guard that raises ValueError if an in-region def is encountered before being cloned. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Roger-luo
added
area: IR
- test_sort_blocks_fixes_scrambled_region: manually scrambles block order, runs SortBlocks, verifies clone produces self-contained region - test_region_clone_raises_on_unsorted_blocks: scrambles blocks on a folded method and asserts Region.clone raises ValueError - Add comment in SortBlocks noting reliance on up-to-date CFG Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@neelay893 can you share your claude prompts in the PR for future reference? the PR itself looks good. |
|
Claude prompts: ❯ there is an issue w/ Region.clone() where the blocks are traversed in index order, not topologically sorted CFG order. This is usually fine, but can cause problems when fixpointing aggressive fold. See kirin_aggressive_fold_mwe.py for an example, and kirin_region_clone_bug.md for a longer description. [it then made a plan which we went back and forth on for a bit; I can try to find the full chat history if this is of interest] ❯ please review the changes ❯ please refactor the RPO traversal into a rewrite pass that mutates the block sequence within the region, such that subsequent traversals will be in the correct order |
Aggressive fold over multiple iterations (fixpoint), can sometimes produce a region with an out of order block sequence (even if the CFG is still well formed). This can cause issues when using `Region.clone()`, where a block using some SSA value is cloned before the block defining it, resulting in the use SSA still pointing to the old object. This PR ~~changes `Region.clone()` to traverse blocks in reverse-post-order to avoid this~~ adds a `SortBlocks` rewrite rule to sort the blocks in each region in reverse-post-order. `SortBlocks` is applied at the end of the `CompactifyRegion` pass. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Aggressive fold over multiple iterations (fixpoint), can sometimes produce a region with an out of order block sequence (even if the CFG is still well formed). This can cause issues when using
Region.clone(), where a block using some SSA value is cloned before the block defining it, resulting in the use SSA still pointing to the old object.This PR
changesadds aRegion.clone()to traverse blocks in reverse-post-order to avoid thisSortBlocksrewrite rule to sort the blocks in each region in reverse-post-order.SortBlocksis applied at the end of theCompactifyRegionpass.