fix(lowering): early_unsafe_panic must keep side-effecting statements - #10033
Conversation
The pass replaces the dead tail of a function (from which return is unreachable) with unsafe_panic, and is meant to preserve side-effecting libfunc calls (debug::print / internal::trace) that must still run first. But it scanned statements forward and truncated AT the first side effect (`truncate(i)`), deleting the very statement it meant to keep. It also only considered the first side effect, dropping later ones in the same block. Scan in reverse and truncate after the last side effect (`i + 1`). Move the test from PreOptimizations to PostBaseline (where this pass actually runs, so print is the inlined extern it detects) and add a regression case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview The Reviewed by Cursor Bugbot for commit 782884c. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
Fixes the
early_unsafe_panicoptimization so that theunsafe_panicinsertion point is placed after the last side-effecting statement in a block, rather than at the statement itself. Previously, the loop iterated forward and pushed the fix at indexi(overwriting the side-effecting statement), and the test harness ran againstPreOptimizationslowering where inlined externs likedebug::printwere not yet visible. Now the loop iterates in reverse, finds the last side-effecting statement, and inserts the early panic ati + 1— preserving the side effect. The test stage is also updated toPostBaselineso that inlined side-effecting externs are present when the optimization runs.Type of change
Please check one:
Why is this change needed?
The previous forward-iterating loop pushed a fix at index
ifor the first side-effecting statement it found, which would replace that statement with anunsafe_paniccall instead of inserting the panic after it. This caused side-effecting statements (e.g.,debug::print) to be dropped from the lowered output. Additionally, the test harness usedPreOptimizationslowering, which does not include inlined externs, so the bug was not caught by the existing test for the side-effect-preservation case.What was the behavior or documentation before?
When a block contained a side-effecting statement followed by dead code (no reachable
return), the optimization replaced the side-effecting statement itself withunsafe_panic, discarding the side effect.What is the behavior or documentation after?
The optimization now inserts
unsafe_panicimmediately after the last side-effecting statement, preserving all side effects. A new test case (Test that a side-effecting statement (debug::print) before the dead tail is preserved) verifies this behavior usingPostBaselinelowering.Related issue or discussion (if any)
N/A
Additional context
The fix also simplifies the conditional logic: instead of checking
ReachableSideEffects::Reachableto early-return and then re-checkingUnreachableinside the loop, the new code destructuresUnreachableupfront and returns early if the state isReachable.