♻️ Relax condition on modifiers#1751
Merged
Merged
Conversation
denialhaag
added
refactor
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
11 tasks
denialhaag
force-pushed
the
more-control
branch
2 times, most recently
from
6911b65 to
7d6476a
Compare
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughRefactors ctrl/inv modifiers so regions receive explicit target/qubit ValueRanges and body callbacks take ValueRange; adds target-alias parsing/printing utilities; converts op TDs and generated APIs to indexed body-unitary access; updates builders, canonicalizations, region inlining, conversions, translation state, verification, and tests. ChangesModifier API and Operation Definitions
Builders and Public APIs
Modifier Implementations & Canonicalizations
Conversion & Lowering
Translation Infrastructure
Verification & Optimizations
Tests & Fixtures
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches✨ Simplify code
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h (2)
903-921:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation example does not match the new signature.
The example shows
builder.ctrl(q0, [&] { builder.x(q1); });but the new signature requires explicittargetsand a callback acceptingValueRange. Update to reflect the new API:builder.ctrl({q0}, {q1}, [&](ValueRange targets) { builder.x(targets[0]); });🤖 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/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h` around lines 903 - 921, Update the doc example for QCProgramBuilder::ctrl to match the new signature ctrl(ValueRange controls, ValueRange targets, const function_ref<void(ValueRange)>& body); — change the C++ and MLIR examples to pass controls and targets as ValueRange (e.g., {q0}, {q1}) and make the callback accept a ValueRange parameter (e.g., [&](ValueRange targets) { /* use targets[0] */ }); so the example shows using targets inside the lambda rather than a no-arg lambda.
923-941:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation example does not match the new signature.
The example shows
builder.inv([&] { builder.s(q0); });but the new signature requires explicitqubitsand a callback acceptingValueRange. Update to reflect the new API:builder.inv({q0}, [&](ValueRange qubits) { builder.s(qubits[0]); });🤖 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/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h` around lines 923 - 941, The doc example for QCProgramBuilder::inv is out of date; update the comment to show the new signature that accepts explicit qubits and a callback taking a ValueRange: replace the lambda-only example with one that calls inv with a braced qubit list (e.g., {q0}) and a lambda taking a ValueRange parameter (e.g., named qubits) that invokes builder.s on qubits[0]; ensure the text and the MLIR example reflect the new ValueRange parameter usage.
🤖 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/include/mlir/Dialect/Utils/Utils.h`:
- Around line 181-194: Add a precondition assertion at the start of
populateMapping to ensure innerQubits.size() == block.getNumArguments() before
indexing innerQubits in the loop: check this invariant (e.g., with assert(...)
or an LLVM-style assertion) referencing the symbols populateMapping,
innerQubits, and block.getNumArguments(), so the function fails fast and clearly
if the sizes mismatch rather than causing undefined behavior when accessing
innerQubits[arg.getArgNumber()].
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 946-952: The remapping uses op.getBody()->getArguments() which
assumes the inv's block args keep the same ordering as the outer targets;
instead iterate over the inv op's operands and use each operand's BlockArgument
index to pick the correct outer qubit. Concretely, replace the loop over
op.getBody()->getArguments() with a loop over op.getOperands(), cast each
operand to a BlockArgument (or otherwise retrieve its getArgNumber()), then push
outerQubits[thatIndex] into innerQubits and set state.targetsIn =
std::move(innerQubits); this preserves subset/reordering like qco.inv(%b,%a)
correctly.
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 126-137: The inlineRegion helper can mis-handle mixed-type
operands when inlining SCF regions; before performing the llvm::zip_equal and
eraseArguments steps in inlineRegion, add explicit checks that the number of
replacementValues matches the number of block arguments from offset and that all
corresponding types are qubit types (e.g., assert or llvm_unreachable with clear
message if any arg.getType() or replacementVal.getType() is not the expected
qubit type); perform these guarded checks in inlineRegion (and analogously at
the other noted sites) so zip_equal assumptions hold and eraseArguments is only
called when the preconditions are satisfied.
In `@mlir/lib/Conversion/QCToQIR/QCToQIR.cpp`:
- Around line 212-215: The decrement and cleanup for control context are
incorrect: don’t unconditionally decrement state.inCtrlOp (which can underflow)
and check the post-decrement value when deciding to clear state.controls. Fix by
only decrementing when state.inCtrlOp > 0 and then test the updated value (e.g.,
use a pre-decrement on state.inCtrlOp or check state.inCtrlOp == 0 after
decrement) before calling state.controls.clear(); update the code paths that
reference inCtrlOp to use state.inCtrlOp so the captured old value isn’t used.
In `@mlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cpp`:
- Around line 615-628: Add an explicit size-check before populating
state.ctrlTargets in the builder.ctrl callback: verify that targetArgs.size() ==
sortedPairs.size() (e.g., with an assert or
llvm::reportFatalInternalError/llvm::errs() + llvm_unreachable) and fail fast if
it doesn't match; then proceed to assign state.ctrlTargets[sortedPairs[i].first]
= targetArgs[i] and run translateOperation as before. This change touches the
builder.ctrl lambda in TranslateQuantumComputationToQC.cpp and ensures the
invariant between targetArgs and sortedPairs is enforced before using those
containers.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 972-973: The test entry QCOTestCase{"inverseTwoX",
MQT_NAMED_BUILDER(twoX), MQT_NAMED_BUILDER(emptyQCO)} incorrectly reuses the
twoX builder; replace the second argument with the inverse-specific builder for
this case (e.g., MQT_NAMED_BUILDER(inverseTwoX) or the project’s dedicated
inverse builder) so that the "inverseTwoX" QCOTestCase uses the inverse-focused
builder instead of MQT_NAMED_BUILDER(twoX).
---
Outside diff comments:
In `@mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h`:
- Around line 903-921: Update the doc example for QCProgramBuilder::ctrl to
match the new signature ctrl(ValueRange controls, ValueRange targets, const
function_ref<void(ValueRange)>& body); — change the C++ and MLIR examples to
pass controls and targets as ValueRange (e.g., {q0}, {q1}) and make the callback
accept a ValueRange parameter (e.g., [&](ValueRange targets) { /* use targets[0]
*/ }); so the example shows using targets inside the lambda rather than a no-arg
lambda.
- Around line 923-941: The doc example for QCProgramBuilder::inv is out of date;
update the comment to show the new signature that accepts explicit qubits and a
callback taking a ValueRange: replace the lambda-only example with one that
calls inv with a braced qubit list (e.g., {q0}) and a lambda taking a ValueRange
parameter (e.g., named qubits) that invokes builder.s on qubits[0]; ensure the
text and the MLIR example reflect the new ValueRange parameter usage.
🪄 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: cca60f16-1ea9-4e24-8a0a-7d503638f8f4
📒 Files selected for processing (33)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Conversion/QCToQIR/QCToQIR.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/QCOps.cppmlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QCO/Transforms/Optimizations/HadamardLifting.cppmlir/lib/Support/IRVerification.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/Conversion/QCToQIR/test_qc_to_qir.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QC/Translation/test_quantum_computation_translation.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.hmlir/unittests/programs/qir_programs.cppmlir/unittests/programs/qir_programs.hmlir/unittests/programs/quantum_computation_programs.cppmlir/unittests/programs/quantum_computation_programs.h
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
11 tasks
…e on `getOutputForInput` Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Member
|
I am still working on this. I just pushed a first batch of changes to get a better PR diff again. |
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
…erOp, CtrlOp, and InvOp Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
approved these changes
Jun 10, 2026
burgholzer
left a comment
burgholzer
left a comment
Member
There was a problem hiding this comment.
Alright. I am through with all of the changes here.
This was a big one. It took a couple of hours to go through everything.
I managed to simplify a couple of concepts here and there while working my way through. Conceptually, this was pretty clean already.
Please have another close look at the changes that I introduced and see whether they make sense. Some of them are pretty nice simplifications I believe.
This is good to go from my side.
Signed-off-by: burgholzer <burgholzer@me.com>
Member
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
mlir/include/mlir/Dialect/Utils/Utils.h (1)
86-145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd Doxygen blocks for the new aliasing helpers.
parseTargetAliasingandprintTargetAliasingare new public header utilities, but their parser/printer contract is only implicit in the implementation right now. Please document both at the declaration site.As per coding guidelines, "MUST use Doxygen-style comments in C++ code."
🤖 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/include/mlir/Dialect/Utils/Utils.h` around lines 86 - 145, Add Doxygen-style documentation for the two new public utilities parseTargetAliasing and printTargetAliasing at their declaration site in the header: describe the purpose, parameters (OpAsmParser& parser, Region& region, SmallVectorImpl<OpAsmParser::UnresolvedOperand>& operands for parseTargetAliasing; OpAsmPrinter& printer, Region& region, OperandRange targetsIn for printTargetAliasing), expected assembly syntax/semantics (format of "(%new = %old, ...)", that parseTargetAliasing binds block entry args of QubitType), return behavior (ParseResult success/failure) and side effects (populates operands and region or prints region), and any invariants (QubitType hard-coded for targets). Ensure comments use Doxygen tags (brief/param/returns) and follow the project's coding guidelines.Source: Coding guidelines
mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (1)
471-484:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPush modifier regions onto
regionStackwhile the callback runs.Unlike
scfFor,scfWhile, andscfIf, these helpers never updateregionStackaround the body. Calls such asmemrefLoadinside actrl/invcallback will still see the parent region and can either reject valid operations or record them under the wrong region.🛠️ Suggested fix
QCProgramBuilder& QCProgramBuilder::ctrl(ValueRange controls, ValueRange targets, const function_ref<void(ValueRange)>& body) { checkFinalized(); - CtrlOp::create(*this, controls, targets, body); + CtrlOp::create(*this, controls, targets, [&](ValueRange bodyTargets) { + regionStack.emplace_back(getInsertionBlock()->getParent()); + body(bodyTargets); + regionStack.pop_back(); + }); return *this; } QCProgramBuilder& QCProgramBuilder::inv(ValueRange qubits, const function_ref<void(ValueRange)>& body) { checkFinalized(); - InvOp::create(*this, qubits, body); + InvOp::create(*this, qubits, [&](ValueRange bodyQubits) { + regionStack.emplace_back(getInsertionBlock()->getParent()); + body(bodyQubits); + regionStack.pop_back(); + }); return *this; }🤖 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/QC/Builder/QCProgramBuilder.cpp` around lines 471 - 484, The ctrl and inv helpers (QCProgramBuilder::ctrl and QCProgramBuilder::inv) create CtrlOp/InvOp but do not push the newly-created operation's region onto the builder's regionStack while executing the body, so nested calls like memrefLoad see the wrong parent region; update both functions to: create the CtrlOp/InvOp (using CtrlOp::create and InvOp::create) but ensure you push the op's region onto regionStack before invoking the body callback and pop it after the callback returns (use the same region stack manipulation mechanics used by scfFor/scfWhile/scfIf so regionStack.back() points to the op's region during body execution).mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp (1)
101-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe single-control
gphasefold now pulls a potentially body-local angle out of the modifier. That replacement is only safe whenthetaalready exists outside the region and there are no other helper ops in the body. With relaxed bodies, it can otherwise generate invalid SSA and silently discard setup ops. Please guard this fold or inline/clone the supporting defs first.🤖 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/QC/IR/Modifiers/CtrlOp.cpp` around lines 101 - 110, The fold that replaces a controlled gphase with a single POp (detecting GPhaseOp via dyn_cast on innerOp and checking op.getNumControls() == 1, then calling rewriter.replaceOpWithNewOp<POp>(..., gPhaseOp.getTheta())) is unsafe when gPhaseOp.getTheta() is a body-local SSA value or the gphase body contains helper ops; fix by first checking that gPhaseOp.getTheta() is defined outside the region and that the gphase body contains no extra helper ops (or clone/inline those supporting defs into the outer region) before performing the replacement, otherwise bail out (return failure()) so you don’t create invalid SSA or drop setup ops.mlir/lib/Conversion/QCToQCO/QCToQCO.cpp (1)
513-524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep walking into modifier regions when discovering SCF-carried state.
The new
!isa<qc::CtrlOp, qc::InvOp>(operation)filter stops the enclosing SCF op from recording memrefs/qubits that are only referenced inside a modifier body. Those values then never become iter_args inConvertSCFForOp/ConvertSCFWhileOp, so a load/extract inside the lowered modifier body falls back to the outer tensor mapping and loses updates across loop or branch boundaries.Based on PR objectives: modifier bodies may now contain arbitrarily many operations, including non-unitary ops.
🤖 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 513 - 524, The code is incorrectly skipping collection for operations that are qc::CtrlOp or qc::InvOp, preventing values used only inside modifier regions from becoming SCF iter_args; remove the "!isa<qc::CtrlOp, qc::InvOp>(operation)" filter so that when an operation has regions (operation.getNumRegions() > 0) you always call collectQubitValuesInsideSCFOps(&operation, state) and merge results into state->regionQubitMap[op] and state->regionRegisterMap[op] (and keep the duplicate-removal using qubitInfoMap) so modifier-region-only qubits/registers are recorded and later turned into iter_args used by ConvertSCFForOp/ConvertSCFWhileOp.
♻️ Duplicate comments (1)
mlir/lib/Conversion/QCOToQC/QCOToQC.cpp (1)
127-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the all-qubit precondition before erasing SCF block arguments.
ConvertQCOSCFForOpandConvertQCOSCFWhileOpstill funnel illegal SCF ops through this helper, but the helper unconditionally replaces and erases every carried block argument afteroffset. If a non-qubit iter-arg slips through, it gets dropped as if it were rewritten qubit state.Based on learnings: “all SCF conversion patterns ... assume and enforce that all operands are qubit types ... add explicit type checks or assertions to reject non-qubit operands.”
🤖 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/QCOToQC/QCOToQC.cpp` around lines 127 - 139, inlineRegion currently unconditionally replaces and erases every carried block argument after offset, which drops non-qubit iter-args; update inlineRegion to verify the precondition that all block arguments being replaced are qubit types before performing replaceAllUsesWith/eraseArguments: iterate over block.getArguments().drop_front(offset), check each argument’s type is the quantum qubit type (e.g., mlir::quantum::QubitType::is or an isQubitType helper), and if any argument is not a qubit either assert/fail fast or return/report a conversion failure so the callers ConvertQCOSCFForOp/ConvertQCOSCFWhileOp do not silently drop non-qubit args; only perform the replacement and block.eraseArguments when the qubit-type check succeeds.Source: Learnings
🤖 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/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 964-966: The getParameters() implementation for CtrlOp should not
fatal; change it to return an empty OperandRange to match getNumParams() == 0
and how qc.inv/CtrlOp variants expose parameters so generic UnitaryOpInterface
iteration works; leave getParameter(size_t i) as a fatal/error for out-of-range
access but ensure getParameters() returns an empty range (consistent with
getNumParams and other dialects).
In `@mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp`:
- Around line 46-75: The current rewrite assumes the outer modifier body
contains only the inner unitary and drops surrounding helper ops; change CtrlOp
rewrite in the block using getSoleBodyUnitary, innerCtrlOp,
utils::getValueFromBlockArgument, merged, rewriter.inlineRegionBefore and
rewriter.eraseOp so it only proceeds when the outer body truly contains just the
single inner operation (e.g., check op.getBody()->getOperations() has exactly
the inner operation plus only an expected terminator) and otherwise bail out
with failure(); alternatively, if you must support multi-op bodies, clone/carry
the full outer body into the new merged region and remap block arguments/usages
(replacing block-arg lookups via utils::getValueFromBlockArgument) before
erasing the outer op so helper ops and dominance are preserved.
---
Outside diff comments:
In `@mlir/include/mlir/Dialect/Utils/Utils.h`:
- Around line 86-145: Add Doxygen-style documentation for the two new public
utilities parseTargetAliasing and printTargetAliasing at their declaration site
in the header: describe the purpose, parameters (OpAsmParser& parser, Region&
region, SmallVectorImpl<OpAsmParser::UnresolvedOperand>& operands for
parseTargetAliasing; OpAsmPrinter& printer, Region& region, OperandRange
targetsIn for printTargetAliasing), expected assembly syntax/semantics (format
of "(%new = %old, ...)", that parseTargetAliasing binds block entry args of
QubitType), return behavior (ParseResult success/failure) and side effects
(populates operands and region or prints region), and any invariants (QubitType
hard-coded for targets). Ensure comments use Doxygen tags (brief/param/returns)
and follow the project's coding guidelines.
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 513-524: The code is incorrectly skipping collection for
operations that are qc::CtrlOp or qc::InvOp, preventing values used only inside
modifier regions from becoming SCF iter_args; remove the "!isa<qc::CtrlOp,
qc::InvOp>(operation)" filter so that when an operation has regions
(operation.getNumRegions() > 0) you always call
collectQubitValuesInsideSCFOps(&operation, state) and merge results into
state->regionQubitMap[op] and state->regionRegisterMap[op] (and keep the
duplicate-removal using qubitInfoMap) so modifier-region-only qubits/registers
are recorded and later turned into iter_args used by
ConvertSCFForOp/ConvertSCFWhileOp.
In `@mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp`:
- Around line 471-484: The ctrl and inv helpers (QCProgramBuilder::ctrl and
QCProgramBuilder::inv) create CtrlOp/InvOp but do not push the newly-created
operation's region onto the builder's regionStack while executing the body, so
nested calls like memrefLoad see the wrong parent region; update both functions
to: create the CtrlOp/InvOp (using CtrlOp::create and InvOp::create) but ensure
you push the op's region onto regionStack before invoking the body callback and
pop it after the callback returns (use the same region stack manipulation
mechanics used by scfFor/scfWhile/scfIf so regionStack.back() points to the op's
region during body execution).
In `@mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp`:
- Around line 101-110: The fold that replaces a controlled gphase with a single
POp (detecting GPhaseOp via dyn_cast on innerOp and checking op.getNumControls()
== 1, then calling rewriter.replaceOpWithNewOp<POp>(..., gPhaseOp.getTheta()))
is unsafe when gPhaseOp.getTheta() is a body-local SSA value or the gphase body
contains helper ops; fix by first checking that gPhaseOp.getTheta() is defined
outside the region and that the gphase body contains no extra helper ops (or
clone/inline those supporting defs into the outer region) before performing the
replacement, otherwise bail out (return failure()) so you don’t create invalid
SSA or drop setup ops.
---
Duplicate comments:
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 127-139: inlineRegion currently unconditionally replaces and
erases every carried block argument after offset, which drops non-qubit
iter-args; update inlineRegion to verify the precondition that all block
arguments being replaced are qubit types before performing
replaceAllUsesWith/eraseArguments: iterate over
block.getArguments().drop_front(offset), check each argument’s type is the
quantum qubit type (e.g., mlir::quantum::QubitType::is or an isQubitType
helper), and if any argument is not a qubit either assert/fail fast or
return/report a conversion failure so the callers
ConvertQCOSCFForOp/ConvertQCOSCFWhileOp do not silently drop non-qubit args;
only perform the replacement and block.eraseArguments when the qubit-type check
succeeds.
🪄 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: dc8e0ca4-d48b-4979-8bc9-39765024f294
📒 Files selected for processing (23)
CHANGELOG.mdmlir/include/mlir/Conversion/ConversionUtils.hmlir/include/mlir/Dialect/QC/IR/QCDialect.hmlir/include/mlir/Dialect/QC/IR/QCInterfaces.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/IR/QCODialect.hmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Conversion/QCToQIR/QCToQIR.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cppmlir/lib/Dialect/QCO/Transforms/Optimizations/HadamardLifting.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/programs/qc_programs.cpp
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mlir/include/mlir/Dialect/Utils/Utils.h (2)
199-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd assertion to guard against out-of-bounds access.
If
qubitis aBlockArgumentwhosegetArgNumber()exceedsqubits.size(), the index access at line 201 causes undefined behavior. This could occur if the block argument comes from a different block than the onequbitsrepresents.🛡️ Proposed fix
inline Value getValueFromBlockArgument(Value qubit, ValueRange qubits) { if (auto blockArg = dyn_cast<BlockArgument>(qubit)) { + assert(blockArg.getArgNumber() < qubits.size() && + "block argument index must be within qubits range"); return qubits[blockArg.getArgNumber()]; } return qubit; }🤖 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/include/mlir/Dialect/Utils/Utils.h` around lines 199 - 204, The function getValueFromBlockArgument currently indexes qubits with blockArg.getArgNumber() without bounds checking; add a guard/assert that blockArg.getArgNumber() < qubits.size() before accessing qubits to avoid out-of-bounds UB (use an llvm/standard assert or a conditional that returns qubit or emits an error), referencing the BlockArgument, getArgNumber(), and qubits.size() in the fix; ensure the code path still returns the original qubit when the check fails or fails fast with a clear diagnostic.
171-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd assertion to validate entry block argument count matches targets.
The loop at line 186 accesses
entryBlock.getArgument(i)assuming the entry block has at leastnumTargetsarguments. If this invariant is violated, the index access causes undefined behavior before any error handling can occur.🛡️ Proposed fix
inline void printTargetAliasing(OpAsmPrinter& printer, Region& region, OperandRange targetsIn) { printer << "("; if (region.empty()) { printer << ") "; printer.printRegion(region, false); return; } auto& entryBlock = region.front(); + assert(entryBlock.getNumArguments() >= targetsIn.size() && + "entry block must have at least as many arguments as targets"); const auto numTargets = targetsIn.size();🤖 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/include/mlir/Dialect/Utils/Utils.h` around lines 171 - 193, The function printTargetAliasing assumes the region's entry block has at least as many arguments as targetsIn; add a runtime assertion at the start of printTargetAliasing (before the loop that calls entryBlock.getArgument(i)) that checks entryBlock.getNumArguments() >= numTargets (use the project's assertion mechanism, e.g., assert or LLVM_DEBUG/llvm_unreachable style) and include a clear message referencing entryBlock and targetsIn length to avoid out-of-bounds access when calling entryBlock.getArgument(i).
🤖 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.
Outside diff comments:
In `@mlir/include/mlir/Dialect/Utils/Utils.h`:
- Around line 199-204: The function getValueFromBlockArgument currently indexes
qubits with blockArg.getArgNumber() without bounds checking; add a guard/assert
that blockArg.getArgNumber() < qubits.size() before accessing qubits to avoid
out-of-bounds UB (use an llvm/standard assert or a conditional that returns
qubit or emits an error), referencing the BlockArgument, getArgNumber(), and
qubits.size() in the fix; ensure the code path still returns the original qubit
when the check fails or fails fast with a clear diagnostic.
- Around line 171-193: The function printTargetAliasing assumes the region's
entry block has at least as many arguments as targetsIn; add a runtime assertion
at the start of printTargetAliasing (before the loop that calls
entryBlock.getArgument(i)) that checks entryBlock.getNumArguments() >=
numTargets (use the project's assertion mechanism, e.g., assert or
LLVM_DEBUG/llvm_unreachable style) and include a clear message referencing
entryBlock and targetsIn length to avoid out-of-bounds access when calling
entryBlock.getArgument(i).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52c8d034-6458-429c-bf68-42281507b759
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp
Member
Author
|
Thanks a lot for all the improvements, @burgholzer! I really like how clean this PR has become despite its size. Given that CodeRabbit is happy as well, I'll set this to auto-merge. 🎉 |
This was referenced Jun 11, 2026
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
2 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 relaxes the condition that modifiers in QC and QCO must have a single unitary operation followed by a
yieldoperation. Modifiers may now contain an arbitrary number of operations (unitary as well as, e.g.,arith.constants).Fixes #1678
Fixes #1727
Checklist
I have added migration instructions to the upgrade guide (if needed).