Repository navigation
feat(ir): move and splice statements, blocks and CFGs in Rewriter - #848
alexcgatea wants to merge 14 commits into
Conversation
Add replace_uses_if, replace_all_uses_except_stmt and replace_all_uses_except_stmts, which redirect only the use sites a predicate accepts. ReplacedUses is recorded only once the old value has no uses left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The event log now only grows while a Rewriter lives, so positions in events() stay valid. Tests track positions with an EventCursor helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moves record MovedStatement; walk_mut rejects moving the visited statement in link-following orders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ore/after Moves every non-terminator statement of a block next to an anchor and redirects its arguments; records SplicedBlock, which walk_mut checks. Drops the planned block move (step 3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moves every block of a CFG after an anchor block in another CFG; records SplicedCfg. walk_mut now records a VisitedPlace (block and CFG) before each visit to check splices against. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
planza-quera
left a comment
There was a problem hiding this comment.
Looks good, though if I'm honest this has become quite hard to review since it's a lot of code that I haven't worked with. I suggest also getting an AI to review it.
| let result = visit(rewriter, stmt); | ||
| Self::check_not_moved_since(rewriter, stmt, place, seen)?; |
There was a problem hiding this comment.
Should we first check that result is an interrupt to end the walk, or check that stmt wasn't moved? The former allows us to move a stmt and end the walk without failing.
There was a problem hiding this comment.
True, but I think in this case we do want the walk to fail because the contract specifies the current statement shouldn't move (regardless of the walk result).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- run the rewrite driver's sweep through walk_mut - check erases and merges of the visited statement on interrupt too - no longer exempt the walk root from the move checks - make walk cursors crate-private - rename ALLOWS_DISPLACING_VISITED to ALLOWS_ERASING_OR_MOVING_VISITED Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…moved Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st helpers Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ebug - share the depth-first cursors' visited-statement check - merge split imports and rewrap the Skip docs - debug-assert that unlink gets a live item from a non-empty list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Side enum replaces the before/after bool in slot_at and its callers - Rewriter::set_terminator replaces duplicated code in merge and split - check_not_moved_since no longer generic over the walk order Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- walk_mut checks erasure and moves (including merges) after each visit - cursors no longer track the visited block; next() is infallible - rename check_visited to check_erasure Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
A couple of iterations of AI review suggested some refactoring such as linking and unlinking statements in a block and blocks in a CFG via a ListItem trait for Statement and Block, and checking the walk contract in one place in walk_mut only. I think the changes are helpful but now there's quite a bit more code change in this PR. |
Summary
Adds the
Rewriteroperations for code motion: moving statements, splicing a block's statements, and splicing a whole CFG. Walks now detect when the statement being visited is moved, and the rewrite driver is held to the same walk contract.New operations
replace_uses_if,replace_all_uses_except_stmt(s)replace_all_usesrestricted to selected use sitesmove_before/move_aftersplice_block_before/splice_block_aftersplice_cfg_afterEach records a new event (
MovedStatement,SplicedBlock,SplicedCfg). Moving a statement into its own bodies, or splicing a block or CFG into itself, is rejected (MovedIntoItself,SplicedIntoItself,CfgSplicedIntoItself). Moving a statement to where it already is (for examplemove_before(a, b)whenais directly beforeb) does nothing and records no event.API changes
drain_eventsis replaced byinto_events(self), so the event log only grows while aRewriterlives, and positions in it stay valid.kirin-ir, a walk isWalkOrder::walk,walk_mutorschedule;WalkCursor, the cursor types andWalkOrder::startare crate-private. A cursor does not check the walk contract (onlywalk_mutdoes, using theRewriter's event log), so every walk that edits now goes throughwalk_mut.WalkOrder::ALLOWS_ERASING_VISITEDis renamedALLOWS_ERASING_OR_MOVING_VISITED.Walk safety
Moving or splicing the statement being visited breaks pre-order, post-order and BFS walks, which find the next statement through the visited one. After each visit,
walk_mutfails withWalkError::VisitedMovedif the statement was:move_before/move_after(from the event log);splice_block_*orsplice_cfg_after(from the event log, with its block or CFG compared before and after the visit, so a terminator left behind by a block splice, or a splice that moves nothing, is fine);The reverse BFS orders, which build their order up front, still allow all of these.
All contract checks now run in one place: right after each visit,
walk_mutchecks erasure (each order's own rule, e.g. BFS allows erasing the visited statement together with an ancestor), then moves, before the cursor steps on. As a result:Interruptincluded: erasing or moving the visited statement and then interrupting is an error too.next()only steps and cannot fail;walkandscheduleno longer discard errors silently.Rewrite driver
RewriteDriver's sweep now runs throughwalk_mut, so a rule that moves, splices or erases the statement being visited where the walk order does not allow it fails the run withDriverError::Walk, as its docs already said. A rule's own error is reported before a walk error. This is temporary: #854 replaces the sweep with a worklist over a snapshot, where moves are allowed.Internal
rewrite/list.rs:link,unlink,split_off, over a smallListItemtrait forStatementandBlock), in the spirit of MLIR'silist_traits. They replace six hand-written copies;split_blockusessplit_off+link.merge_blocksandsplit_blockhand a terminator to its new block through one helper,Rewriter::set_terminator.Sideenum (Before/After) instead of a bool.