✨ Add SCF conversions between QC and QCO dialects#1638
Open
li-mingbao wants to merge 67 commits intomunich-quantum-toolkit:mainfrom
Open
✨ Add SCF conversions between QC and QCO dialects#1638li-mingbao wants to merge 67 commits intomunich-quantum-toolkit:mainfrom
li-mingbao wants to merge 67 commits intomunich-quantum-toolkit:mainfrom
Conversation
added 30 commits
April 9, 2026 14:24
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: li-mingbao <74404929+li-mingbao@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
970-986:⚠️ Potential issue | 🟠 MajorEnsure
beforeBodyreturns the same values yielded byscf.condition.
scfCondition()builds the actual terminator operands, butscfWhile()uses the callback return value for tracking and for the after-region arguments. If these differ, the IR and builder state diverge.Suggested consistency check
// Construct the body const auto& results = body(blockArgs); if (results.size() != initArgs.size()) { llvm::reportFatalUsageError( "scf.while body must return exactly one value per iter arg"); } // Create the yield operation, the scf.condition operation is created // separately if (createYield) { scf::YieldOp::create(*this, results); + } else { + auto conditionOp = + llvm::dyn_cast_or_null<scf::ConditionOp>(block->getTerminator()); + if (!conditionOp) { + llvm::reportFatalUsageError( + "scf.while before body must create an scf.condition terminator"); + } + if (!llvm::equal(conditionOp.getArgs(), results)) { + llvm::reportFatalUsageError( + "scf.while before body must return the same values yielded by " + "scf.condition"); + } } return results; };Also applies to: 1048-1063
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h`:
- Around line 180-200: Update the memrefLoad documentation to state the
same-region precondition: in QCProgramBuilder::memrefLoad(Value memref, Value
index) document that the index Value must be defined in the same region where
the memref.load is emitted (e.g., the nested SCF region) because the builder
tracks loaded indices per-region and using an index from a different region can
break those assumptions; reference the memrefLoad signature explicitly and add a
short sentence to the doc comment explaining this requirement and its rationale.
In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h`:
- Around line 1235-1257: The qcoIf documentation still describes only qubit
inputs and the example's else lambda omits the args parameter; update the doc
for qcoIf to state that initArgs may include both qubit Values and tensor Values
(and that tensors will be implicitly extracted/inserted as described), clarify
that the returned ValueRange must match input qubit/tensor types, and fix the
example so both thenBody and elseBody lambdas take ValueRange args (e.g.,
[&](ValueRange args) -> llvm::SmallVector<Value>) and use args indices
consistently in each branch; reference the qcoIf signature, the
thenBody/elseBody lambdas, and ValueRange/SmallVector<Value> in the text.
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 723-730: The matchAndRewrite in ConvertQCOYieldOp currently only
checks for scf::IfOp parent and emits scf.yield; change the condition to treat
both qco::IfOp and scf::IfOp parents as requiring scf.yield (i.e., check if
op->getParentOp() is isa<qco::IfOp>() || isa<scf::IfOp>()), and keep emitting
scf::YieldOp in that branch and qc::YieldOp otherwise so inlined qco.if regions
get the correct scf.yield terminator.
In `@mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp`:
- Around line 528-569: The then/else branch lambdas in QCProgramBuilder::scfIf
duplicate the same region setup/teardown (InsertionGuard,
setInsertionPointToStart, regionStack push/pop and scf::YieldOp); extract that
into a small local helper lambda (e.g. buildRegion) that captures *this and
takes the user callback (llvm::function_ref<void()>) and returns a lambda
matching OpBuilder/Location signature, then call scf::IfOp::create with
buildRegion(thenBody) and buildRegion(elseBody) (or only buildRegion(thenBody)
when elseBody is empty), ensuring regionStack push/pop and scf::YieldOp remain
exactly once in the helper.
- Around line 122-139: Insert a call to checkFinalized() at the start of
QCProgramBuilder::memrefLoad so the builder rejects mutations after finalize;
specifically, add checkFinalized() before any use of getInsertionBlock() or
memref::LoadOp::create, then proceed with the existing region/loadedQubits
checks and creation of the load op (ensure
loadedQubits[region][memref].insert(index) remains after creating the
memref::LoadOp).
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 1019-1035: The code calls updateQubitValueTracking(thenResult,
elseArgs) before verifying thenResult's arity, which can trigger zip_equal
assertions if thenBody returned the wrong number of values; move the arity
checks so you first verify thenResult.size() == initArgs.size() (and later
elseResult.size() == initArgs.size() before yielding) and only then call
updateQubitValueTracking and YieldOp::create; specifically, check the sizes of
thenResult and elseResult relative to initArgs after computing them (from
thenBody and elseBody) and before invoking updateQubitValueTracking or
YieldOp::create to prevent premature assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a5e45a7-3b7b-4be1-8df2-4b6e519446c2
📒 Files selected for processing (8)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp
Contributor
Author
|
Hey @burgholzer, |
Member
Hey 👋🏻 |
Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp (1)
246-281:⚠️ Potential issue | 🟠 MajorRestore branch-yield/result validation in
IfOp::verify.Line 140 replaces the
qco.ifwith the selected branch terminator operands. Right nowverify()never checks thatthenYield()andelseYield()produce the same number and types of values as the op results, so malformed IR can pass verification and then breakRemoveStaticConditionor later rewrites on an arity/type mismatch.💡 Suggested fix
if (numInputQubits != numOutputQubits) { return emitOpError("Operation must return the same number of qubits as the " "number of input qubits."); } + auto thenYieldTypes = thenYield().getOperandTypes(); + auto elseYieldTypes = elseYield().getOperandTypes(); + if (thenYieldTypes.size() != numOutputQubits || + elseYieldTypes.size() != numOutputQubits) { + return emitOpError("Both regions must yield the same number of values as " + "the operation returns."); + } + for (auto [thenType, elseType, resultType] : + llvm::zip_equal(thenYieldTypes, elseYieldTypes, getResultTypes())) { + if (thenType != resultType || elseType != resultType) { + return emitOpError("Both regions must yield the same types as the " + "operation results."); + } + } for (auto [inputQubitType, outputQubitType] : llvm::zip_equal(inputQubits.getTypes(), outputQubits.getTypes())) { if (inputQubitType != outputQubitType) { return emitOpError("Operation must return the same qubit types as its " "input qubit types.");Based on learnings:
qco.ifguarantees both then and else regions exist and each is terminated byqco.yield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp` around lines 246 - 281, The verify() in IfOp must also validate branch yields: ensure thenYield() and elseYield() exist and that both produce the same number of values and types that match the operation results (getResults()) and input qubits (getQubits())—i.e., check thenYield()->getNumOperands() == elseYield()->getNumOperands() == getResults().size() and that each yielded type equals the corresponding result type (and/or input qubit type where appropriate); if any mismatch emitOpError with a clear message; update IfOp::verify to perform these checks using the existing symbols thenYield(), elseYield(), getResults(), getQubits() and return failure on mismatch so downstream passes like RemoveStaticCondition can rely on arity/type consistency.
♻️ Duplicate comments (1)
mlir/lib/Conversion/QCToQCO/QCToQCO.cpp (1)
1253-1282:⚠️ Potential issue | 🟠 MajorReject or remap existing SCF carried block arguments before erasing the old op.
Line 1266 only rewires the induction variable, and the
scf.whilerewrite never rewires the old before/after block arguments at all. If this pass ever sees a QCscf.for/scf.whilewith pre-existing qubit/tensor carried state, the spliced body will keep using the old region arguments andrewriter.eraseOp(op)will invalidate them. Please either RAUW the old carried block arguments to the newly created block arguments or fail fast when the source op already has carriers/results outside the builder-generated zero-carrier shape.Based on learnings, the rewrite is intentionally scoped to all-qubit SCF ops, but that invariant does not remap already-existing qubit/qtensor region arguments.
Also applies to: 1333-1374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` around lines 1253 - 1282, The rewrite currently only replaces the induction variable and then erases the old op, which leaves any pre-existing carried block arguments (qubits/qtensors) in the spliced body referring to invalid region arguments; update the logic after splicing (around where newForOp is created and dstBlock/dstBlock.getArguments() are used) to RAUW each old block argument from op.getRegion().front() to the corresponding new block argument in newForOp.getRegion().front() (use dstBlock.getArguments().drop_front(1).take_back(numQubits) and .take_front(numRegisters) for mapping), or if the source op contains carriers/results beyond the builder-generated zero-carrier shape, bail out with a clear failure instead; apply the same replacement/failure pattern to the analogous scf.while rewrite path referenced in the comment (the block around lines 1333-1374).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h`:
- Around line 1260-1343: The documentation for scfFor and scfWhile is out of
date: it still states iter-args are qubits only and the scfFor example mixes
builder names (uses b.* calls while declaring builder) which is misleading now
that tensor containers are supported; update the prose to say iter-args/initArgs
may be qubits or tensor containers (tensor<...!qco.qubit>) and that
qtensorExtract/qtensorInsert are used when an iter-arg is a tensor, fix the
example code snippets to use the same builder variable name (e.g.,
builder.qtensorExtract / builder.h / builder.qtensorInsert) and provide a
parallel scfWhile example that shows tensor initArgs as well; also add a short
note that qco.if was similarly extended to accept tensor iter-args so callers
know the API change (reference symbols: scfFor, scfWhile, qtensorExtract,
qtensorInsert, qco.h, and qco.if).
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 1059-1060: Update the dialect documentation strings to reflect
that qco.yield and qco.if signatures (and the related scf variants) accept
LinearType, not just qubits: change descriptions around the definitions for
qco.yield and qco.if (and the other affected spots noted near lines 1246-1249)
to state that init/iter arguments may be LinearType including tensor containers
like tensor<...x!qco.qubit>; mention that tensor containers are supported for
scf.for, scf.while (scfWhile) and qco.if/scf.if (qcoIf) so the generated docs
accurately show tensor<...x!qco.qubit> support. Ensure any wording that
currently says “qubit-only” is replaced with “LinearType (including
tensor<...x!qco.qubit>)” and keep the summaries consistent with the changed ins
types (Variadic<LinearType>:$targets).
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 763-775: The current lowering for scf::ForOp (and similarly for
the scf::If case around lines 812–823) unconditionally erases loop results and
replaces uses with adaptor.getInitArgs(), which is only correct if each carried
slot preserves positional aliasing across every scf.yield/scf.condition; add a
verification or fallback: before calling inlineRegion and rewriter.replaceOp in
the scf::ForOp lowering (symbols: scf::ForOp::create, inlineRegion,
rewriter.replaceOp, adaptor.getInitArgs()), scan the loop body and all
scf.yield/scf.condition sites to ensure each yielded value for index i is the
same physical container/operand as the corresponding init arg (or provably
equivalent); if the invariant fails, either emit a diagnostic and return failure
(or skip lowering) or adjust the lowering to preserve distinct results (i.e.,
keep the scf::ForOp result values instead of replacing them). Ensure a short doc
comment near the lowering and/or add a verifier check that enforces this
positional-aliasing invariant for loop carriers.
In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp`:
- Around line 203-207: The printer currently emits the result signature by
iterating the then-region entry block args
(getThenRegion().front().getArgumentTypes()), which can diverge from the op's
actual results; change the printing to use the op's explicit result types via
getResultTypes() (i.e., replace the call to
getThenRegion().front().getArgumentTypes() with getResultTypes() in the block
that prints the "-> (...)" clause so interleaveComma prints the op's real result
types).
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 154-194: The test IfOpParser (TEST_F QCOTest, IfOpParser) only
covers scalar !qco.qubit paths; add a new tensor-based case that exercises
qco.if/qco.yield with tensor<...x!qco.qubit> init/iter args to catch
parser/cleanup regressions. Update the mlirCode snippet used to parse (the
module string inside IfOpParser or add a new TEST_F) to allocate/insert/extract
a tensor<1x!qco.qubit> and use qco.if with qubits(%arg0 = %q2) where %arg0 has
tensor<1x!qco.qubit>, and ensure qco.yield returns that tensor form in both
branches; also update or add a matching reference builder (use
QCOProgramBuilder::build with a new named builder like simpleIfTensor or extend
MQT_NAMED_BUILDER(simpleIf) to produce a tensor-typed result) and keep the same
verification/cleanup/assertion sequence (parseSourceString, verify,
runQCOCleanupPipeline, areModulesEquivalentWithPermutations) to validate tensor
support.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp`:
- Around line 246-281: The verify() in IfOp must also validate branch yields:
ensure thenYield() and elseYield() exist and that both produce the same number
of values and types that match the operation results (getResults()) and input
qubits (getQubits())—i.e., check thenYield()->getNumOperands() ==
elseYield()->getNumOperands() == getResults().size() and that each yielded type
equals the corresponding result type (and/or input qubit type where
appropriate); if any mismatch emitOpError with a clear message; update
IfOp::verify to perform these checks using the existing symbols thenYield(),
elseYield(), getResults(), getQubits() and return failure on mismatch so
downstream passes like RemoveStaticCondition can rely on arity/type consistency.
---
Duplicate comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 1253-1282: The rewrite currently only replaces the induction
variable and then erases the old op, which leaves any pre-existing carried block
arguments (qubits/qtensors) in the spliced body referring to invalid region
arguments; update the logic after splicing (around where newForOp is created and
dstBlock/dstBlock.getArguments() are used) to RAUW each old block argument from
op.getRegion().front() to the corresponding new block argument in
newForOp.getRegion().front() (use
dstBlock.getArguments().drop_front(1).take_back(numQubits) and
.take_front(numRegisters) for mapping), or if the source op contains
carriers/results beyond the builder-generated zero-carrier shape, bail out with
a clear failure instead; apply the same replacement/failure pattern to the
analogous scf.while rewrite path referenced in the comment (the block around
lines 1333-1374).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ecbe3b4-1e8b-483b-8b98-f1a0cd268e70
📒 Files selected for processing (12)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QCO/IR/SCF/IfOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cpp
Member
|
@li-mingbao, I wanted to bring this PR up to speed after the merge of #1673. Unfortunately, my activity triggered CodeRabbit, which then found issues unrelated to the rebase. 😕 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (1)
129-137:⚠️ Potential issue | 🔴 CriticalReturn the
Biton the non-error path.For valid indices, this function reaches the end without returning a value. Any call to
register[i]is therefore undefined.Suggested fix
if (index < 0 || index >= size) { const std::string msg = "Bit index " + std::to_string(index) + " out of bounds for register '" + name + "' of size " + std::to_string(size); llvm::reportFatalUsageError(msg.c_str()); - return {.registerName = name, .registerSize = size, .registerIndex = index}; } + return {.registerName = name, .registerSize = size, .registerIndex = index}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp` around lines 129 - 137, The operator ClassicalRegister::operator[](const int64_t index) const currently only returns on the error path; for valid indices it falls off without returning a Bit. Fix by returning a Bit constructed from this register for the non-error path: in QCProgramBuilder::ClassicalRegister::operator[] create and return a Bit with .registerName = name, .registerSize = size, and .registerIndex = index (the same shape as the error return uses) when index is within bounds so every control flow returns a Bit.
♻️ Duplicate comments (3)
mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h (1)
1217-1258:⚠️ Potential issue | 🟡 MinorDocs still read as qubit-only for the new tensor-capable SCF builders.
qcoIf,scfFor, andscfWhilenow takeValueRange initArgs, but the prose still says “qubit values” in multiple places, and thescfForexample switches frombuildertob. That leaves the public header out of sync with the API right where the new tensor-carrying control-flow support is introduced. Based on learnings: tensor containers (tensor<...x!qco.qubit>) are supported as initArgs/iter-args forscf.for,scf.while, andqco.ifas of PR#1638.📝 Suggested doc fix
- * Constructs an if operation that takes a bool Value and a range of qubit - * values that are used in the then/else region of this operation. The qubit - * values are passed down as block arguments to each region. Qubits that were + * Constructs an if operation that takes a bool Value and a range of qubit or + * tensor-of-qubit values that are used in the then/else region of this + * operation. The input values are passed down as block arguments to each + * region. Qubits that were * extracted from a tensor that is used as an argument for this operation are * automatically inserted before the operation is constructed. ... - * Constructs a scf.for operation with the given loop boundaries and stepsize - * and a range of qubit values for its iter args. + * Constructs a scf.for operation with the given loop boundaries and step size + * and a range of qubit or tensor-of-qubit values for its iter args. ... - * auto [t0, q0] = b.qtensorExtract(iterArgs[0], iv); - * auto q1 = b.h(q0); - * auto insert = b.qtensorInsert(q1, t0, iv); + * auto [t0, q0] = builder.qtensorExtract(iterArgs[0], iv); + * auto q1 = builder.h(q0); + * auto insert = builder.qtensorInsert(q1, t0, iv); ... - * Constructs a scf.while with a range of qubit values for its iter args. + * Constructs a scf.while with a range of qubit or tensor-of-qubit values for + * its iter args.Also applies to: 1260-1342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h` around lines 1217 - 1258, The header docs incorrectly describe initArgs as "qubit values" and contain an inconsistent example variable (uses "b" instead of "builder"); update the documentation for qcoIf (and the corresponding scfFor/scfWhile blocks) to state that initArgs can be qubit values or tensor containers (e.g., tensor<... x !qco.qubit>), adjust the prose to mention tensor-carrying init/iter-args, and fix the example to consistently use "builder" (not "b"); ensure the updated descriptions appear for qcoIf, scfFor, and scfWhile (also apply the same fixes around the other commented range mentioned).mlir/lib/Conversion/QCOToQC/QCOToQC.cpp (1)
762-773:⚠️ Potential issue | 🟠 MajorDon’t erase loop-carried results unless slot identity is enforced.
Both loop lowerings replace the original
scf.for/scf.whileresults withadaptor.getInitArgs()/adaptor.getInits(). That is only semantics-preserving if each carried slot is guaranteed to denote the same physical qubit/container in the same position across everyscf.yield/scf.condition. A loop that permutes carriers or yields a different resource in a slot will be silently mislowered here.Either reject such loops explicitly (assert/diagnostic/verifier) or keep the carriers/results threaded through the lowered SCF op instead of dropping them.
Also applies to: 815-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp` around lines 762 - 773, The lowering currently drops loop-carried results by replacing the original scf.for/scf.while op with adaptor.getInitArgs() (see scf::ForOp::create, inlineRegion, and rewriter.replaceOp(op, adaptor.getInitArgs())), which is only valid if each carried slot preserves identity; fix by preserving/threading the loop-carried results instead of discarding them: after inlining the region, capture the values produced by the inlined terminator(s) (the scf.yield/scf.condition results) and replace the original op with those yielded values (wiring them through the newFor/newWhile results), or alternatively add an explicit verifier/assertion that each yielded slot is identical to the corresponding init slot before allowing the current replacement; apply the same change for the scf.while lowering code paths referenced near the other occurrence.mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
968-999:⚠️ Potential issue | 🔴 CriticalUse the
scf.conditionoperands as the single source of truth.Lines 993-994 thread the vector returned from
beforeBody()into theafterregion, but Line 1072 builds the actualscf.conditionfrom an independentyieldedValueslist. If those two sets ever differ, the builder state no longer matches the IR and region-local values can leak or be validated against the wrong carried state.This looks like the same root issue that was flagged earlier, and it still appears present in the current implementation. The fix needs to bind bookkeeping to the operands passed to
scfCondition()itself, or derive both the terminator and the threaded values from one shared result.Also applies to: 1057-1072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp` around lines 968 - 999, The builder currently threads the vector from beforeBody() into the after region (beforeResults) but constructs the scf.condition and/or uses a separate yieldedValues list, causing divergence between region-local values and the condition operands; fix this by making the scf.condition operands the single source of truth: when you create the scf::ConditionOp (scfCondition()) use the exact vector returned by createBody(beforeBlock, beforeBody, ...) (beforeResults) and reuse that same vector for building the after region and for any scf::YieldOp creation and calls to updateQubitValueTracking (i.e., replace any independent yieldedValues with beforeResults/afterResults so createBody, scf::ConditionOp creation, scf::YieldOp creation, and updateQubitValueTracking all reference the same Value vector such that beforeResults -> scf::ConditionOp operands -> after region inputs remain identical).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 1200-1203: The terminator replacements (e.g., the block where
currentModifierFrame(state) is used, resolveMappedQubits(state, operation,
frame.qcQubits), rewriter.replaceOpWithNewOp<qco::YieldOp>(op, targets),
popModifierFrame(state)) should be rewritten to replace the terminator in place:
set the rewriter insertion point to the existing terminator (call
rewriter.setInsertionPoint(op)), create the new qco::YieldOp (or the
corresponding terminator used at the other sites) at that insertion point, then
erase the old terminator op with rewriter.eraseOp(op) instead of
replaceOpWithNewOp; apply this same pattern for the other two locations
referenced (the replaceOpWithNewOp sites around lines 1493–1495 and 1531–1533).
- Around line 1250-1263: The new scf::ForOp created by scf::ForOp::create(...)
includes an auto-generated body with a terminator, so before splicing srcBlock
into newForOp’s body you must remove that auto-generated terminator/block to
avoid duplicate scf.yield terminators; locate the creation and subsequent
splicing around scf::ForOp::create(...), newForOp.getRegion().front(),
srcBlock/dstBlock and either erase the auto-created body block or erase its
terminator (the auto-generated scf.yield) on dstBlock before doing
dstBlock.getOperations().splice(...), then proceed with assignMappedTensors,
assignMappedQubits and the rewriter.replaceAllUsesWith(...) call.
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 229-269: Detect and reject duplicate non-qubit region operands
before mutating tensor tracking: before accessing validTensors[initArg] or
calling updateTensorTracking inside the loop over initArgs, check whether the
same tensor SSA value appears earlier in initArgs/updatedArgs (e.g., keep a
SmallPtrSet<Value> seenTensors or test updatedArgs.contains(initArg)); if a
duplicate non-qubit tensor is found, emit the same linear-use error
(llvm::reportFatalUsageError) and return instead of proceeding. Also avoid using
operator[] on validTensors for presence checks—use find/contains or at() so you
don't accidentally default-insert a TensorInfo for the second occurrence; only
mutate tracking (updateTensorTracking, validQubits.erase, currentTensor
assignment, updatedArgs.emplace_back) after the duplicate check passes.
---
Outside diff comments:
In `@mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp`:
- Around line 129-137: The operator ClassicalRegister::operator[](const int64_t
index) const currently only returns on the error path; for valid indices it
falls off without returning a Bit. Fix by returning a Bit constructed from this
register for the non-error path: in
QCProgramBuilder::ClassicalRegister::operator[] create and return a Bit with
.registerName = name, .registerSize = size, and .registerIndex = index (the same
shape as the error return uses) when index is within bounds so every control
flow returns a Bit.
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h`:
- Around line 1217-1258: The header docs incorrectly describe initArgs as "qubit
values" and contain an inconsistent example variable (uses "b" instead of
"builder"); update the documentation for qcoIf (and the corresponding
scfFor/scfWhile blocks) to state that initArgs can be qubit values or tensor
containers (e.g., tensor<... x !qco.qubit>), adjust the prose to mention
tensor-carrying init/iter-args, and fix the example to consistently use
"builder" (not "b"); ensure the updated descriptions appear for qcoIf, scfFor,
and scfWhile (also apply the same fixes around the other commented range
mentioned).
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 762-773: The lowering currently drops loop-carried results by
replacing the original scf.for/scf.while op with adaptor.getInitArgs() (see
scf::ForOp::create, inlineRegion, and rewriter.replaceOp(op,
adaptor.getInitArgs())), which is only valid if each carried slot preserves
identity; fix by preserving/threading the loop-carried results instead of
discarding them: after inlining the region, capture the values produced by the
inlined terminator(s) (the scf.yield/scf.condition results) and replace the
original op with those yielded values (wiring them through the newFor/newWhile
results), or alternatively add an explicit verifier/assertion that each yielded
slot is identical to the corresponding init slot before allowing the current
replacement; apply the same change for the scf.while lowering code paths
referenced near the other occurrence.
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 968-999: The builder currently threads the vector from
beforeBody() into the after region (beforeResults) but constructs the
scf.condition and/or uses a separate yieldedValues list, causing divergence
between region-local values and the condition operands; fix this by making the
scf.condition operands the single source of truth: when you create the
scf::ConditionOp (scfCondition()) use the exact vector returned by
createBody(beforeBlock, beforeBody, ...) (beforeResults) and reuse that same
vector for building the after region and for any scf::YieldOp creation and calls
to updateQubitValueTracking (i.e., replace any independent yieldedValues with
beforeResults/afterResults so createBody, scf::ConditionOp creation,
scf::YieldOp creation, and updateQubitValueTracking all reference the same Value
vector such that beforeResults -> scf::ConditionOp operands -> after region
inputs remain identical).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b043b258-2ea4-491c-9d42-fadbc8505b84
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
973-1003:⚠️ Potential issue | 🔴 CriticalDrive
scf.whiletracking from the actualscf.conditionoperands.The builder currently seeds the after-region from
beforeBody's return value and maps loop results fromafterBody's return value, but in MLIR the values passed toscf.conditionare the false-exit results and the after-region may not execute at all. That lets builder tracking diverge from the IR wheneverbeforeBody()returns something different fromscfCondition(...), or when the loop exits after mutating values in the before-region.Also applies to: 1062-1077
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp` around lines 973 - 1003, The current builder seeds the after-region and loop-result tracking from beforeBody/afterBody return values (createBody + updateQubitValueTracking), which can diverge from the IR; instead locate the scf.condition operation tied to the scf.while (the scf::ConditionOp created for the loop), extract its operand Values (the false-exit results passed to the condition) and use those operands as the source for updateQubitValueTracking for the whileOp results and as the inputs to the after-region mapping, i.e., change the code around createBody/use of beforeResults/afterResults so that updateQubitValueTracking(afterOperandsFromCondition, whileOp->getResults()) and the call to createBody(afterBlock, afterBody, /*initArgs=*/conditionOperands, true) drive the tracking rather than using beforeBody/afterBody return values; keep createBody(beforeBlock, ...) behavior for before-region execution but do not seed the loop-result mapping from beforeBody results.mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h (1)
1278-1283:⚠️ Potential issue | 🟡 MinorFix inconsistent builder variable name in example.
The example uses
builder.scfFor(...)on the outer call butb.qtensorExtract,b.h,b.qtensorInsertinside the lambda. This inconsistency can confuse users sincebis never defined. Usebuilderconsistently throughout.📝 Suggested documentation fix
* `@par` Example: * ```c++ * builder.scfFor(lb, ub, step, initArgs, [&](Value iv, ValueRange iterArgs) * -> SmallVector<Value> { - * auto [t0, q0] = b.qtensorExtract(iterArgs[0], iv); - * auto q1 = b.h(q0); - * auto insert = b.qtensorInsert(q1, t0, iv); + * auto [t0, q0] = builder.qtensorExtract(iterArgs[0], iv); + * auto q1 = builder.h(q0); + * auto insert = builder.qtensorInsert(q1, t0, iv); * return {insert}; * });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h` around lines 1278 - 1283, The example uses builder.scfFor(...) but then refers to an undefined variable b inside the lambda; replace b with builder for consistency so the calls to qtensorExtract, h, and qtensorInsert use the same builder instance (i.e., change b.qtensorExtract -> builder.qtensorExtract, b.h -> builder.h, b.qtensorInsert -> builder.qtensorInsert inside the lambda passed to builder.scfFor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 1259-1264: The moved ops still reference the old scf.for iter-arg
block arguments, so before erasing the source loop (op) you must remap each
srcBlock argument to the corresponding dstBlock argument; iterate over
srcBlock.getNumArguments(), for each index get srcArg = srcBlock.getArgument(i)
and dstArg = dstBlock.getArgument(i) and call
rewriter.replaceAllUsesWith(srcArg, dstArg) (using the same rewriter used for
induction var RAUW of op.getInductionVar() -> newForOp.getInductionVar()), then
proceed with splicing and erasing the original op so no uses or invalid defs
remain.
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h`:
- Around line 1278-1283: The example uses builder.scfFor(...) but then refers to
an undefined variable b inside the lambda; replace b with builder for
consistency so the calls to qtensorExtract, h, and qtensorInsert use the same
builder instance (i.e., change b.qtensorExtract -> builder.qtensorExtract, b.h
-> builder.h, b.qtensorInsert -> builder.qtensorInsert inside the lambda passed
to builder.scfFor).
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 973-1003: The current builder seeds the after-region and
loop-result tracking from beforeBody/afterBody return values (createBody +
updateQubitValueTracking), which can diverge from the IR; instead locate the
scf.condition operation tied to the scf.while (the scf::ConditionOp created for
the loop), extract its operand Values (the false-exit results passed to the
condition) and use those operands as the source for updateQubitValueTracking for
the whileOp results and as the inputs to the after-region mapping, i.e., change
the code around createBody/use of beforeResults/afterResults so that
updateQubitValueTracking(afterOperandsFromCondition, whileOp->getResults()) and
the call to createBody(afterBlock, afterBody, /*initArgs=*/conditionOperands,
true) drive the tracking rather than using beforeBody/afterBody return values;
keep createBody(beforeBlock, ...) behavior for before-region execution but do
not seed the loop-result mapping from beforeBody results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0f00bfc-f63e-44b8-9fc7-8c4089073add
📒 Files selected for processing (7)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/QCOOps.cpp
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.
Description
This PR adds support for the conversion of the
scfoperationsscf.while,scf.for,scf.ifbetween theQCand theQCOdialect. This allows the conversion of programs with nonlinear controlflow.This PR is the revamped version of the #1396 PR to integrate the newest changes.
Changes compared to the PR and new features:
scfoperations.qtensor.castoperation first.scf.ifis converted intoqco.ifqco.ifalso supports tensors of qubits now as input types.qco.yieldalso supports tensors of qubits now as input types.Checklist:
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.