[python] Pass the loop-carried arguments to a bare break or continue - #5177
[python] Pass the loop-carried arguments to a bare break or continue#5177udsy19 wants to merge 1 commit into
Conversation
A `break` or `continue` written directly in a `for` body, rather than
nested inside an `if`, emitted its `cc.break` / `cc.continue` terminator
with an empty operand list. Every terminator in a `cc.loop` body region
has to forward that region's block arguments, so the kernel failed to
compile:
error: 'cc.loop' op along control flow edge from Operation
cc.continue to Region NVIDIA#2: region branch point has 0 operands, but
region successor needs 1 inputs
RuntimeError: could not compile code for '...'
`createForLoop` establishes the invariant itself: the implicit
fall-through terminator it appends uses `bodyBlock.arguments`, and that
is exactly the tuple `pushForBodyStack` records. The `isInIfStmtBlock()`
branch of `visit_Break` / `visit_Continue` already forwards it; only the
other branch hardcoded `[]`. Hoist the binding above the `if` and use it
in both branches.
`while` loops are unaffected either way -- `visit_While` builds the loop
with no block arguments, so the forwarded list is empty and the emitted
IR is unchanged.
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
schweitzpgi
left a comment
There was a problem hiding this comment.
Yeah, that's a bug in the bridge. In fact, we should get rid of the premature optimizations in the bridge of it trying to thread variables around as values. The design is to have the compiler do that itself so it's done correctly.
|
Thanks @schweitzpgi — and agreed on the larger point about the bridge threading values around. This PR does not touch that design; it only passes the loop-carried arguments that CI has never run on it. Could you post |
| # CHECK-DAG: %[[VAL_44:.*]] = cc.undef i64 | ||
| # CHECK-DAG: %[[VAL_45:.*]] = quake.alloca !quake.veq<4> | ||
| # CHECK: %[[VAL_46:.*]]:2 = cc.loop while ((%[[VAL_47:.*]] = %[[VAL_42]], %[[VAL_48:.*]] = %[[VAL_44]]) -> (i64, i64)) { | ||
| # CHECK: %[[VAL_49:.*]] = arith.cmpi slt, %[[VAL_47]], %[[VAL_43]] : i64 | ||
| # CHECK: cc.condition %[[VAL_49]](%[[VAL_47]], %[[VAL_48]] : i64, i64) | ||
| # CHECK: } do { | ||
| # CHECK: ^bb0(%[[VAL_50:.*]]: i64, %[[VAL_51:.*]]: i64): | ||
| # CHECK: %[[VAL_52:.*]] = quake.extract_ref %[[VAL_45]]{{\[}}%[[VAL_50]]] : (!quake.veq<4>, i64) -> !quake.ref | ||
| # CHECK: quake.ry (%[[VAL_40]]) %[[VAL_52]] : (f64, !quake.ref) -> () | ||
| # CHECK: cc.continue %[[VAL_50]], %[[VAL_50]] : i64, i64 | ||
| # CHECK: } step { | ||
| # CHECK: ^bb0(%[[VAL_53:.*]]: i64, %[[VAL_54:.*]]: i64): | ||
| # CHECK: %[[VAL_55:.*]] = arith.addi %[[VAL_53]], %[[VAL_41]] : i64 | ||
| # CHECK: cc.continue %[[VAL_55]], %[[VAL_54]] : i64, i64 |
There was a problem hiding this comment.
The CHECK lines expect the loop to carry two i64 values plus a cc.undef i64. The kernel in the test has no break, so the loop carries only the induction variable. The real IR is a single argument loop with no cc.undef.
Running the file's own RUN line fails.
A
breakorcontinuewritten directly in aforbody, rather than nested inside anif, emittedits
cc.break/cc.continueterminator with an empty operand list, so the kernel failed tocompile:
The same happens for
break, in nested loops, when iterating aqvector, and in purely quantumkernels with no classical return value.
What this changes
Every terminator in a
cc.loopbody region has to forward that region's block arguments.createForLoopestablishes the invariant itself — the implicit fall-through terminator it appendsuses
bodyBlock.arguments, and that is exactly the tuplepushForBodyStackrecords. TheisInIfStmtBlock()branch ofvisit_Break/visit_Continuealready forwards it; only the otherbranch hardcoded
[]. Hoisting the binding above theifand using it in both branches is anet-zero-line change:
The resulting terminator has the same shape
python/tests/mlir/ast_break.pyalready asserts for theif-nested path.whileloops are unaffected either way:visit_Whilebuilds the loop with no block arguments, sothe forwarded list is empty. I confirmed the printed module for a
whileloop with a barecontinueand with a barebreakis byte-identical before and after this change, as is the modulefor each of the two kernels the existing tests in these files already cover.
This is the same class of arity mismatch as #1682, fixed in #1693 — that fix corrected
inForBodyStack[0]→inForBodyStack[-1]in theisInIfStmtBlock()branch and left the otherbranch emitting
[].Testing
python/tests/mlir/ast_break.pygainstest_bare_breakandpython/tests/mlir/ast_continue.pygains
test_bare_continue, each aforloop whosebreak/continueis not nested in anif,with a CHECK block in the files' existing style asserting the terminator now carries the
loop-carried operands. Both tests also invoke the kernel, exercising the JIT path that raised the
RuntimeError. Both fail before this change and pass after it.There was no coverage of this branch before: both files place the statement under an
if, and anAST scan of
python/finds no@cudaq.kernelanywhere in the tree with abreakorcontinuethat is not nested in an
if.Validation
The change was applied to the
ast_bridge.pyof an installedcudaq0.15.1 wheel(
cuda_quantum_cu13, Python 3.13, macOS 26.5.1 arm64), exercised end to end, and the wheel restoredafterwards. Before, all eight
for-loop forms failed to compile; after, every one matches CPython(
3,1,6,2,5, and the expected measurement distributions), and every case that workedbefore is unchanged.
Fixes #5176