✨ Add SCF conversions between QC and QCO dialects#1638
✨ Add SCF conversions between QC and QCO dialects#1638li-mingbao wants to merge 77 commits intomunich-quantum-toolkit:mainfrom
Conversation
|
Hey @burgholzer, |
Hey 👋🏻 |
Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@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. 😕 |
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
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
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
# Conflicts: # CHANGELOG.md
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @li-mingbao for all the work on this PR! 🙏🏼
I finally found some time to get to this.
I pushed a couple of commits that cleaned up a couple of smaller details and improved small bits and pieces.
I also have a handful of smaller comments left, which you will find inline. These should be quick to address and I believe this should be ready to go in afterwards.
I have the feeling that the conversions between QC and QCO have accumulated quite some tracking code and data structures that feel a bit much. And I am wondering whether some of the respective code could be simplified. However, these are all concerns for a potential follow-up PR.
Let's get this one in sooner rather than later 😄
| * scf.for %iv = %lb to %ub step %step { | ||
| * %q0 = memref.load %memref[%iv] : memref<3x!qc.qubit> | ||
| * qc.h %q0 : !qc.qubit | ||
| * scf.yield | ||
| * } |
There was a problem hiding this comment.
scf.for has an implicit terminator trait, so I would have guessed that the scf.yield is never printed in valid IR. Am I wrong here?
This also holds for the other builder examples down below.
There was a problem hiding this comment.
You are right for scf.for and scf.if. I removed these instances from the docstrings.
In the scf.while operation the yield operation is still printed.
| //===--------------------------------------------------------------------===// | ||
|
|
||
| /** | ||
| * @brief Construct a scf.for operation |
There was a problem hiding this comment.
I am pretty sure that the correct spelling is "... an scf.<...>" because the pronounciation of scf starts with an "es" sound. Applies everywhere across the entire PR.
There was a problem hiding this comment.
I should have corrected all instances of this.
| parseIfOpAliasing(OpAsmParser& parser, Region& thenRegion, Region& elseRegion, | ||
| SmallVectorImpl<OpAsmParser::UnresolvedOperand>& operands) { | ||
| // Parse the qubits keyword | ||
| if (parser.parseKeyword("qubits")) { |
There was a problem hiding this comment.
Is "qubits" still the right keyword here? Especially since this may now also contain qtensor values?
There was a problem hiding this comment.
I am not sure what a good word it would be so I decided to replace them with a generic args keyword to represent both qubits and qtensors here. This is inline with other scf operations like iter_args for scf.for.
| // Automatically deallocate all still-allocated tensors | ||
| for (auto& [tensor, tensorInfo] : validTensors) { | ||
| for (auto& [tensor, tensorInfo] : sortedTensors) { | ||
| auto currentTensor = tensor; | ||
| // Filter out qubits belonging to this tensor | ||
| for (auto& [qubit, qubitInfo] : qubitsByRegister[tensorInfo.regId]) { | ||
| auto indexValue = constantFromScalar(*this, getLoc(), qubitInfo.regIndex); | ||
| currentTensor = | ||
| qtensor::InsertOp::create(*this, qubit, currentTensor, indexValue) | ||
| .getResult(); | ||
| currentTensor = qtensor::InsertOp::create(*this, qubit, currentTensor, |
There was a problem hiding this comment.
There was some sorting of the tensor values here before. I typically avoided such sorting in the conversions as it is not strictly necessary for equivalence and costs time. However, I noticed that one of the tests would sporadically (very rarely fail) specifically the QCOTest.IfOpParser test. Could it be that the module verifier is not yet flexible enough to catch these differences? If so, can we extend it so that this is supported?
There was a problem hiding this comment.
I think I added the sorting since the IfOpParser test would fail sometimes because it creates 2 different registers and the deallocation order of the registers was not deterministic. So the areModulesEquivalentWithPermutations function would fail.
| target.addDynamicallyLegalDialect<scf::SCFDialect>([](Operation* op) { | ||
| // Some types are not converted yet so QC and QCO types have to be checked | ||
| auto isQubitType = [](Type t) { | ||
| return TypeSwitch<Type, bool>(t) | ||
| .Case<qc::QubitType, qco::QubitType>([](auto) { return true; }) | ||
| .Case<MemRefType>([](MemRefType t) { | ||
| return llvm::isa<qc::QubitType>(t.getElementType()); | ||
| }) | ||
| .Case<RankedTensorType>([](RankedTensorType t) { | ||
| return llvm::isa<qco::QubitType>(t.getElementType()); | ||
| }) | ||
| .Default([](auto) { return false; }); | ||
| }; | ||
|
|
||
| return !llvm::any_of(op->getOperandTypes(), isQubitType); | ||
| }); |
There was a problem hiding this comment.
I have to say that this also confuses me a little bit. Are we missing something elsewhere that makes this necessary?
burgholzer
dismissed
denialhaag’s stale review
Dismissing Daniel's review as stale for now.
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 (3)
mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp (1)
246-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReinstate branch-yield checks in
IfOp::verify().
verify()now only ties block arguments and op results back to the input list. It no longer checks thatthenYield()andelseYield()have the same arity and types as theqco.ifresults, so malformed IR can pass verification and later break the constant-folding path on Lines 133-141 or the QCO→QC lowering, which assumes the carried values line up.Suggested fix
LogicalResult IfOp::verify() { const auto& inputQubits = getQubits(); const auto numInputQubits = inputQubits.size(); const auto& outputQubits = getResults(); const auto numOutputQubits = outputQubits.size(); @@ if (numInputQubits != numOutputQubits) { return emitOpError("Operation must return the same number of qubits as the " "number of input qubits."); } + if (thenYield().getOperands().size() != numOutputQubits || + elseYield().getOperands().size() != numOutputQubits) { + return emitOpError("Both regions must yield the same number of values as " + "the operation returns."); + } 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."); } } + for (auto [yieldType, resultType] : + llvm::zip_equal(thenYield().getOperands().getTypes(), + outputQubits.getTypes())) { + if (yieldType != resultType) { + return emitOpError( + "Then-region yield types must match the operation result types."); + } + } + for (auto [yieldType, resultType] : + llvm::zip_equal(elseYield().getOperands().getTypes(), + outputQubits.getTypes())) { + if (yieldType != resultType) { + return emitOpError( + "Else-region yield types must match the operation result types."); + } + } SmallPtrSet<Value, 4> uniqueQubitsIn; for (auto qubit : inputQubits) { if (!uniqueQubitsIn.insert(qubit).second) { return emitOpError("Input qubits must be unique."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp` around lines 246 - 281, IfOp::verify() removed checks that thenYield() and elseYield() match the op results and input qubits; restore verification so that thenYield() and elseYield() both exist, have the same arity as getResults() (and as getQubits()/thenBlock()->getNumArguments()), and that each corresponding yielded type equals the corresponding getResults() type (and input qubit type). Also ensure thenYield() and elseYield() have identical types to each other (same arity and per-index type equality) and emitOpError() with a clear message when any of these conditions fail so malformed IR cannot pass verification.mlir/lib/Conversion/QCToQCO/QCToQCO.cpp (2)
245-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-assert on unsupported qubit-memref boundaries.
This helper still aborts when a qubit-bearing memref has no seeded tensor mapping. That is reachable for function/block arguments or call edges carrying
memref<...x!qc.qubit>: func-boundary conversion is still out of scope here, soConvertMemRefLoadOpcan hitlookupMappedTensor()before any mapping exists and crash instead of producing a clean conversion failure.Based on learnings, tensor containers are supported for
scf.if/scf.whilein this PR, butfuncoperations remain restricted to scalar qubit types only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` around lines 245 - 259, The helper lookupMappedTensor currently aborts via assert when findRegionLocalMap returns no mapping, which crashes during func/block boundary cases (e.g., memref<...x!qc.qubit>) before conversion can fail cleanly; change lookupMappedTensor to not hard-assert—have it return a null/empty Value (e.g., Value()) when tensorMap/tensorValue are missing so callers can detect absence, then update callers such as ConvertMemRefLoadOp (and any users relying on lookupMappedTensor/currentModifierFrame) to check the returned Value and emit a proper conversion failure or diagnostic instead of relying on an assertion; keep use of findRegionLocalMap and currentModifierFrame but remove the assert and propagate the empty result.
1116-1127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRescan cloned modifier regions to populate SCF-use maps.
collectQubitValuesInsideSCFOps()is called once at the start and keysregionQubitMap/regionRegisterMapbyOperation*, butcloneRegionBefore()creates new operation instances for every nestedscf.*op inside the cloned region. Those cloned SCF ops therefore have no entries in the pre-computed maps and appear empty to the legality predicate, remaining legal and unconverted. Nested SCF operations carrying qubits or registers insideqc.ctrl/qc.invbodies will not be rewritten.♻️ Localized fix
Add a rescan after cloning in both
ConvertQCCtrlOp::matchAndRewrite(line 1118) andConvertQCInvOp::matchAndRewrite(line 1168):auto& dstRegion = qcoOp.getRegion(); rewriter.cloneRegionBefore(op.getRegion(), dstRegion, dstRegion.end()); + collectQubitValuesInsideSCFOps(qcoOp.getOperation(), &state);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` around lines 1116 - 1127, The precomputed maps (regionQubitMap/regionRegisterMap populated by collectQubitValuesInsideSCFOps) reference original SCF ops, but rewriter.cloneRegionBefore(...) creates new SCF op instances so the cloned SCF bodies are missing from those maps; after cloning the region in ConvertQCCtrlOp::matchAndRewrite and ConvertQCInvOp::matchAndRewrite (immediately after rewriter.cloneRegionBefore(op.getRegion(), dstRegion, dstRegion.end())), re-run collectQubitValuesInsideSCFOps on the cloned region to repopulate regionQubitMap/regionRegisterMap for the new operations so nested scf.* ops carrying qubits/registers are recognized by the legality predicate before proceeding (i.e., call the same scan helper with the newly-cloned ops/region).
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
967-984:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrive
scf.whilebookkeeping from the emittedscf.condition.
createBody()threads thebeforeBodyreturn value into theafterregion, but the real loop-carried state is whateverscfCondition()put on the terminator. If those ever differ, the builder updatesvalidQubits/validTensorsfor the wrong values and diverges from the IR.🔧 Proposed fix
auto createBody = [&](Block* block, function_ref<SmallVector<Value>(ValueRange)> body, ValueRange innerInitArgs, bool createYield) -> SmallVector<Value> { auto blockArgs = block->getArguments(); // Update the qubit values to the block args updateQubitValueTracking(innerInitArgs, blockArgs); // Construct the body const auto& results = body(blockArgs); if (results.size() != innerInitArgs.size()) { llvm::reportFatalUsageError( "scf.while body must return exactly one value per iter arg"); } if (createYield) { scf::YieldOp::create(*this, results); - } - return results; + return SmallVector<Value>(results.begin(), results.end()); + } + + auto condOp = dyn_cast<scf::ConditionOp>(block->getTerminator()); + if (!condOp) { + llvm::reportFatalUsageError( + "scf.while beforeBody must terminate with scf.condition"); + } + if (condOp.getArgs().size() != innerInitArgs.size()) { + llvm::reportFatalUsageError( + "scf.condition must yield exactly one value per iter arg"); + } + return SmallVector<Value>(condOp.getArgs().begin(), condOp.getArgs().end()); };Also applies to: 1048-1064
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp` around lines 967 - 984, The loop body builder createBody currently uses the body() return (the `results`/`beforeBody`) to update qubit/tensor tracking and to emit the scf::YieldOp, but the true loop-carried values come from the terminator produced by scfCondition(); update createBody to accept and use the values returned by scfCondition (the condition terminator results) when calling updateQubitValueTracking and when creating the scf::YieldOp (i.e., thread the scfCondition terminator values into the `after` region instead of using the body() results), so validQubits/validTensors are updated from the scfCondition outputs rather than from `results`/`beforeBody`. Ensure you preserve the size checks (results.size() vs innerInitArgs.size()) but base bookkeeping and the yielded values on the scfCondition outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 851-852: In ConvertQCOIfOp and ConvertQCOSCFConditionOp replace
uses of op.getCondition() with the remapped operand adaptor.getCondition() when
constructing the new SCF ops (e.g. the scf::IfOp::create call in ConvertQCOIfOp
and the scf::condition creation in ConvertQCOSCFConditionOp), and
restore/un-comment the adaptor parameter in ConvertQCOSCFConditionOp so you can
call adaptor.getCondition(); this ensures the recreated scf ops use the adapted
(converted) SSA value produced by the OpAdaptor rather than the original
op.getCondition().
In `@mlir/unittests/programs/qco_programs.cpp`:
- Around line 2077-2088: Extend the ifOneQubitOneTensor test to include a
non-trivial else branch for b.qcoIf that returns the same mixed qubit+tensor
result types as the then branch (use qtensorExtract/qtensorInsert on the tensor
operand in the else path and perform an operation on the qubit operand in that
branch), and add assertions after lowering to verify the physical qubit identity
is preserved across then/else (i.e., the qubit Value produced by both branches
refers to the same underlying resource); update the usage sites in
ifOneQubitOneTensor (references: qcoIf, qtensorExtract, qtensorInsert,
allocQubitRegister, measure) so both branches return matching SmallVector shapes
and include checks that compare the qubit Values for identity after the
transformation.
---
Outside diff comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 245-259: The helper lookupMappedTensor currently aborts via assert
when findRegionLocalMap returns no mapping, which crashes during func/block
boundary cases (e.g., memref<...x!qc.qubit>) before conversion can fail cleanly;
change lookupMappedTensor to not hard-assert—have it return a null/empty Value
(e.g., Value()) when tensorMap/tensorValue are missing so callers can detect
absence, then update callers such as ConvertMemRefLoadOp (and any users relying
on lookupMappedTensor/currentModifierFrame) to check the returned Value and emit
a proper conversion failure or diagnostic instead of relying on an assertion;
keep use of findRegionLocalMap and currentModifierFrame but remove the assert
and propagate the empty result.
- Around line 1116-1127: The precomputed maps (regionQubitMap/regionRegisterMap
populated by collectQubitValuesInsideSCFOps) reference original SCF ops, but
rewriter.cloneRegionBefore(...) creates new SCF op instances so the cloned SCF
bodies are missing from those maps; after cloning the region in
ConvertQCCtrlOp::matchAndRewrite and ConvertQCInvOp::matchAndRewrite
(immediately after rewriter.cloneRegionBefore(op.getRegion(), dstRegion,
dstRegion.end())), re-run collectQubitValuesInsideSCFOps on the cloned region to
repopulate regionQubitMap/regionRegisterMap for the new operations so nested
scf.* ops carrying qubits/registers are recognized by the legality predicate
before proceeding (i.e., call the same scan helper with the newly-cloned
ops/region).
In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp`:
- Around line 246-281: IfOp::verify() removed checks that thenYield() and
elseYield() match the op results and input qubits; restore verification so that
thenYield() and elseYield() both exist, have the same arity as getResults() (and
as getQubits()/thenBlock()->getNumArguments()), and that each corresponding
yielded type equals the corresponding getResults() type (and input qubit type).
Also ensure thenYield() and elseYield() have identical types to each other (same
arity and per-index type equality) and emitOpError() with a clear message when
any of these conditions fail so malformed IR cannot pass verification.
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 967-984: The loop body builder createBody currently uses the
body() return (the `results`/`beforeBody`) to update qubit/tensor tracking and
to emit the scf::YieldOp, but the true loop-carried values come from the
terminator produced by scfCondition(); update createBody to accept and use the
values returned by scfCondition (the condition terminator results) when calling
updateQubitValueTracking and when creating the scf::YieldOp (i.e., thread the
scfCondition terminator values into the `after` region instead of using the
body() results), so validQubits/validTensors are updated from the scfCondition
outputs rather than from `results`/`beforeBody`. Ensure you preserve the size
checks (results.size() vs innerInitArgs.size()) but base bookkeeping and the
yielded values on the scfCondition outputs.
🪄 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: 385c3c14-c66e-4b6c-bc5a-6d696f32d1bc
📒 Files selected for processing (14)
CHANGELOG.mdmlir/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.cppmlir/unittests/programs/qco_programs.h
| void ifOneQubitOneTensor(QCOProgramBuilder& b) { | ||
| auto q = b.allocQubitRegister(1); | ||
| auto t0 = b.allocQubitRegister(1); | ||
| auto q0 = b.h(q[0]); | ||
| auto [measuredQubit, measureResult] = b.measure(q0); | ||
| b.qcoIf(measureResult, {measuredQubit, t0.value}, [&](ValueRange qubits) { | ||
| auto innerQubit0 = b.x(qubits[0]); | ||
| auto [t1, innerQubit1] = b.qtensorExtract(qubits[1], 0); | ||
| auto innerQubit2 = b.x(innerQubit1); | ||
| auto t2 = b.qtensorInsert(innerQubit2, t1, 0); | ||
| return SmallVector{innerQubit0, t2}; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a non-trivial else-branch variant for mixed qubit/tensor qco.if.
This helper only exercises the then region. The new lowering drops qco.if results and relies on both branches preserving the same underlying resource, so a mixed qubit+tensor case with an active else branch would give much better coverage for the new tensor support.
Based on learnings: "for scf.if with qubit operands, both then and else branches yield the same physical qubits (resources) after conversion ... Add checks (assertions or tests) to verify qubit identity is preserved across branches when performing the transformation."
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 2077-2077: The function 'ifOneQubitOneTensor' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/programs/qco_programs.cpp` around lines 2077 - 2088, Extend
the ifOneQubitOneTensor test to include a non-trivial else branch for b.qcoIf
that returns the same mixed qubit+tensor result types as the then branch (use
qtensorExtract/qtensorInsert on the tensor operand in the else path and perform
an operation on the qubit operand in that branch), and add assertions after
lowering to verify the physical qubit identity is preserved across then/else
(i.e., the qubit Value produced by both branches refers to the same underlying
resource); update the usage sites in ifOneQubitOneTensor (references: qcoIf,
qtensorExtract, qtensorInsert, allocQubitRegister, measure) so both branches
return matching SmallVector shapes and include checks that compare the qubit
Values for identity after the transformation.
3 participants
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.