[core] Document the StackFramePrealloc / addLowerToCFG pairing invariant - #5495
Open
AnitaGeorge404 wants to merge 5 commits into
Open
AnitaGeorge404 wants to merge 5 commits into
AnitaGeorge404 wants to merge 5 commits into
Conversation
Record, at the one place in the compiler that pairs these two passes, why they must never be invoked separately: doing so (running StackFramePrealloc over still-structured cc.scope/cc.if/cc.loop control flow instead of the lowered CFG it analyzes) was tried during review and reproducibly broke dynamic/list-returning `cudaq.run`/`run_async` kernels and mid-circuit-measurement kernels, by letting per-iteration allocations get hoisted and shared across loop iterations instead of getting a fresh stack slot each time. The pairing itself (`addLowerToCFGAndCleanup`, used at every codegen pipeline call site) already encodes the fix and is already pinned by the `PIPELINE` FileCheck block in cudaq/test/Optimizer/atomic_quantum_region_codegen.qke, so this change adds no new behavior; it makes the invariant legible at the point most likely to be edited without that context.
addPipelineTranslateToIQMJson hand-inlined addLowerToCFG followed by StackFramePrealloc instead of using the addLowerToCFGAndCleanup() helper every other codegen pipeline uses for this pair. That is a second, independent place the two passes could be split apart without anyone noticing -- exactly the failure mode NVIDIA#5272 documents for the QIR codegen pipeline. Route it through the shared helper so the pairing is enforced by reuse, not convention. The pass order is unchanged (lower-to-cfg immediately followed by stack-frame-prealloc); the only behavioral difference is the trailing canonicalizer pass the helper always adds, matching every other call site and immediately followed here by combine-quantum-allocations' own canonicalizer, so it is a no-op cleanup pass, not a new transformation.
Collaborator
|
Do you have a reproducer for the issue referenced? |
khalatepradnya
requested changes
Sep 28, 2026
khalatepradnya
left a comment
khalatepradnya
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for working on this, @AnitaGeorge404.
This PR does not resolve #5272, so please remove Closes #5272 from the description.
#5272 asks for StackFramePrealloc to stay in createCommonTargetCodegenPipeline, and for the failures that this placement causes to be fixed. That was the outcome of the #5252 review thread that led to the issue.
The new comment does the opposite.
Root cause: addLowerToCFG's own cc.scope lowering (RewriteScope in LowerToCFG.cpp) unconditionally wraps any scope with a classical allocation in an llvm.stacksave/llvm.stackrestore pair, since a lowered CFG has no structured construct left to express "free this memory when the scope exits". StackFramePrealloc's job of stripping the premature restore for a dynamically sized allocation that must survive past its scope (a value returned from a loop, a mid-circuit measurement result) can only happen once those stacksave/stackrestore calls exist, i.e. after addLowerToCFG has run. That is why the experiment recorded in NVIDIA#5272 -- keep StackFramePrealloc where it was, move only addLowerToCFG -- broke 22 tests: it removed the invocation that actually performs this cleanup, without providing any replacement for it. Fix: restore StackFramePrealloc to its original position in createCommonTargetCodegenPipeline (hoisting loop-invariant classical allocations while control flow is still structured, which is what the common pipeline needs it for), while leaving the addLowerToCFG delay from NVIDIA#5252 and the paired post-addLowerToCFG StackFramePrealloc invocation (addLowerToCFGAndCleanup) completely untouched, since that is the invocation load-bearing for the stacksave/stackrestore cleanup described above. The pass now runs twice, at two points that do different, non-overlapping jobs: the early run only ever hoists fixed-size allocations (a dynamically sized allocation is always pinned in place, regardless of when the pass runs), so it cannot interfere with the late run's cleanup of stacksave/stackrestore pairs that do not exist until addLowerToCFG creates them. Updated the PIPELINE FileCheck block in atomic_quantum_region_codegen.qke to assert stack-frame-prealloc now appears before combine-quantum-alloc in the common pipeline, in addition to its existing, unchanged assertion that it also runs immediately after convert-to-cfg further down.
Author
|
@khalatepradnya Thanks for the clarification. I’ve removed the |
Collaborator
|
Right. StackFramePrealloc is no longer dependent on the IR being in CFG form. We need to fix the bug(s), not reverse course. |
Collaborator
|
I suggest closing this PR, since it was about documenting something that isn't true, and starting with a fresh PR. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Record, at the one place in the compiler that pairs these two passes, why they must never be invoked separately: doing so (running StackFramePrealloc over still-structured cc.scope/cc.if/cc.loop control flow instead of the lowered CFG it analyzes) was tried during review and reproducibly broke dynamic/list-returning
cudaq.run/run_asynckernels and mid-circuit-measurement kernels, by letting per-iteration allocations get hoisted and shared across loop iterations instead of getting a fresh stack slot each time.The pairing itself (
addLowerToCFGAndCleanup, used at every codegen pipeline call site) already encodes the fix and is already pinned by thePIPELINEFileCheck block incudaq/test/Optimizer/atomic_quantum_region_codegen.qke, so this change adds no new behavior; it makes the invariant legible at the point most likely to be edited without that context.