Fix invalid SPIRV from continue inside switch inside for loop#10301
Fix invalid SPIRV from continue inside switch inside for loop#10301jkwak-work merged 1 commit intoshader-slang:masterfrom
continue inside switch inside for loop#10301Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHandle continue targets when eliminating multi-level breaks: loop headers are captured as IRLoop, continue blocks are recognized and treated as exit points when distinct from loop targets, and continue blocks are removed from exit-block sets after region processing to preserve structured control flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6f5c55f to
2188a47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 282e06c7-f085-4f90-8559-0e44e6d10e10
📒 Files selected for processing (2)
source/slang/slang-ir-eliminate-multilevel-break.cpptests/bugs/nested-switch-continue-in-loop.slang
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
…hader-slang#9585, shader-slang#10198) For a for-loop with a switch containing a `continue`, the IR has this shape: loop(target=%body, break=%after, continue=%incr) %body: switch(x, break=%post_switch, ...) case 0: unconditionalBranch(%incr) <- 'continue' case 1: unconditionalBranch(%post_switch) <- 'break' %post_switch: unconditionalBranch(%incr) <- normal post-switch flow %incr: i++; unconditionalBranch(%body) <- back-edge %after: ... <- loop break The `continue` inside the switch branches to %incr (the loop's continueBlock). Because continueBlock (%incr) != targetBlock (%body) in a for-loop, this is a branch that exits the switch region to reach an exit block of the enclosing loop region -- a multi-level branch that must be transformed for valid SPIRV. The bug: populateExitBlocks() stored continueBlock but did not add it to region->exitBlocks, so mapExitBlockToRegion never mapped %incr to the loop region. gatherInfo() therefore never flagged the branch from case 0 to %incr as a multi-level branch, needsContinueElimination stayed false, and the raw IR branch reached the SPIRV emitter -- producing unstructured control flow. The fix: add continueBlock to region->exitBlocks whenever continueBlock != targetBlock (i.e. for for-loops). This makes %incr visible in mapExitBlockToRegion, gatherInfo() detects the multi-level branch, and eliminateContinueBlocksInFunc() is called to wrap the loop body in an inner breakable region. After that transformation the `continue` becomes a break from the inner region -- a valid SPIRV structured exit to its merge block -- and the outer loop handles the actual iteration. Also fix a stack leak: after processing a loop region, pop continueBlock from the global exitBlocks stack unconditionally. For for-loops it is already in info.exitBlocks and removed by the existing loop; for while-loops (where continueBlock == targetBlock and is not in info.exitBlocks) it was pushed to the global stack but never popped, which could affect sibling constructs. Fixes shader-slang#9585, shader-slang#10198.
73b06c8 to
a90cb7c
Compare
|
Having a trouble with CI. |
For a for-loop with a switch containing a
continue, the IR has this shape:The
continueinside the switch branches to %incr (the loop's continueBlock).Because continueBlock (%incr) != targetBlock (%body) in a for-loop, this is a
branch that exits the switch region to reach an exit block of the enclosing loop
region -- a multi-level branch that must be transformed for valid SPIRV.
The bug: populateExitBlocks() stored continueBlock but did not add it to
region->exitBlocks, so mapExitBlockToRegion never mapped %incr to the loop
region. gatherInfo() therefore never flagged the branch from case 0 to %incr as
a multi-level branch, needsContinueElimination stayed false, and the raw IR
branch reached the SPIRV emitter -- producing unstructured control flow.
The fix: add continueBlock to region->exitBlocks whenever continueBlock !=
targetBlock (i.e. for for-loops). This makes %incr visible in
mapExitBlockToRegion, gatherInfo() detects the multi-level branch, and
eliminateContinueBlocksInFunc() is called to wrap the loop body in an inner
breakable region. After that transformation the
continuebecomes a break fromthe inner region -- a valid SPIRV structured exit to its merge block -- and the
outer loop handles the actual iteration.
Also fix a stack leak: after processing a loop region, pop continueBlock from
the global exitBlocks stack unconditionally. For for-loops it is already in
info.exitBlocks and removed by the existing loop; for while-loops (where
continueBlock == targetBlock and is not in info.exitBlocks) it was pushed to
the global stack but never popped, which could affect sibling constructs.
Fixes #9585, #10198.