♻️ Relax condition on modifiers#1751
Open
denialhaag wants to merge 17 commits into
Open
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
📝 WalkthroughWalkthroughThis PR refactors QC/QCO modifier APIs to pass explicit target/qubit ValueRanges into builder callbacks and replaces single-body-unitary accessors with indexed getNumBodyUnitaries()/getBodyUnitary(i); it updates utilities, op defs, builders, canonicalizations, lowering/translation, verification, and tests accordingly. ChangesModifier API Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes 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.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/QC/IR/Modifiers/InvOp.cpp (1)
341-350:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winForward the qubit results when erasing an empty
qc.inv.An empty
qc.invis still an identity operation on its qubit results.eraseOp(op)drops that mapping and will leave any users of theInvOpresults dangling; this should forward the inputs to the outputs instead.💡 Suggested fix
- rewriter.eraseOp(op); + rewriter.replaceOp(op, op.getOperands());🤖 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/InvOp.cpp` around lines 341 - 350, The rewrite currently erases an empty InvOp which drops the mapping from its input qubits to its results; in EraseEmptyInv::matchAndRewrite (for InvOp where op.getNumBodyUnitaries() == 0) replace the erase with forwarding the op's qubit operands to its results using the PatternRewriter replace API (e.g., call rewriter.replaceOp(op, /*forward op qubit operands/results*/ op.getQubits() or the appropriate operand accessor) instead of rewriter.eraseOp(op)) so any users of the InvOp results remain connected.mlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cpp (1)
619-643:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
compoundControlsafter leaving the controlled compound.
state.compoundControlsis populated for the currentCompoundOperationand never restored. After the first controlled compound, later unrelated operations on the same control qubits will have those controls silently skipped bygetControls().🔧 Suggested fix
+ auto previousCompoundControls = state.compoundControls; for (const auto& [control, _] : compoundOp.getControls()) { state.compoundControls.insert(control); } @@ builder.ctrl(controls, targets, [&](ValueRange targetArgs) { state.inCtrlOp = true; @@ state.targetArgs.clear(); state.inCtrlOp = false; }); + state.compoundControls = std::move(previousCompoundControls);🤖 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/Translation/TranslateQuantumComputationToQC.cpp` around lines 619 - 643, The code inserts controls into state.compoundControls for compoundOp but never restores it, causing later operations to see stale controls; fix by saving the original state.compoundControls (or record the inserted controls) before the loop over compoundOp.getControls(), then after the builder.ctrl lambda completes (after state.inCtrlOp = false and state.targetArgs.clear()), restore state.compoundControls to the saved value (or erase only the inserted controls) so compoundControls only reflects the current CompoundOperation; reference state.compoundControls, compoundOp.getControls(), builder.ctrl(..., [&](ValueRange){...}), state.inCtrlOp, and state.targetArgs to locate where to save and restore.mlir/lib/Conversion/QCToQIR/QCToQIR.cpp (1)
870-884:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve nested control contexts instead of rejecting them.
This now fails valid QC IR that still exists in the test-program surface (
nestedCtrl,tripleNestedCtrl,ctrlInvSandwich,nestedCtrlTwoinmlir/unittests/programs/qc_programs.cpp). The previous state model handled nested controls by stacking control context; replacing that withnotifyMatchFailureregresses QC→QIR lowering for those programs.🤖 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/QCToQIR/QCToQIR.cpp` around lines 870 - 884, The code currently rejects nested CtrlOps; instead preserve nested control contexts by saving and restoring the current control state around the inline and erase steps: remove the notifyMatchFailure for nested controls, store oldInCtrl = state.inCtrlOp and oldControls = state.controls, then update state.inCtrlOp (e.g. state.inCtrlOp += op.getNumBodyUnitaries() or push the new value) and state.controls (append or push adaptor.getControls()), call rewriter.inlineBlockBefore(&op.getRegion().front(), op, adaptor.getTargets()), then restore state.inCtrlOp = oldInCtrl and state.controls = oldControls before returning and finally rewriter.eraseOp(op); this keeps nested control stacking while inlining.
♻️ Duplicate comments (1)
mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp (1)
959-964:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemap nested
invtargets from the op operands, not the body-arg indices.This still rebuilds
state.targetsInby walking the inner block arguments, so a reorderedqco.inv(%b = %arg1, %a = %arg0)insideqco.ctrlis remapped back to%arg0, %arg1. The gates lowered inside thatinvwill then act on the wrong outer qubits.💡 Suggested fix
- for (auto arg : op.getBody()->getArguments()) { - innerQubits.push_back(outerQubits[arg.getArgNumber()]); + for (auto operand : op.getQubitsIn()) { + auto outerArg = dyn_cast<BlockArgument>(operand); + assert(outerArg && + "nested qco.inv operands inside qco.ctrl must be block arguments"); + innerQubits.push_back(outerQubits[outerArg.getArgNumber()]); }🤖 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/QCOToJeff/QCOToJeff.cpp` around lines 959 - 964, The current remapping rebuilds state.targetsIn by iterating op.getBody()->getArguments(), which uses inner block-arg indices and reorders targets incorrectly for operands like qco.inv(%b=%arg1, %a=%arg0); change the remapping to iterate op.getOperands() instead and for each operand locate the corresponding outer qubit (either by using operand.getArgNumber() if it's a BlockArgument or by matching the operand Value against entries in outerQubits) and push that outer qubit into innerQubits, then assign state.targetsIn = std::move(innerQubits); update the code around state.targetsIn, innerQubits, op.getBody()->getArguments() to use op.getOperands() lookup so the mapping follows the op operand order.
🤖 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/QCOToJeff/QCOToJeff.cpp`:
- Around line 883-887: The pass currently rejects CtrlOp/InvOp when
op.getNumBodyUnitaries() != 1; change the guard in the QCO→jeff lowering (the
check using op.getNumBodyUnitaries() and the rewriter.notifyMatchFailure call)
to allow zero or multiple-body cases that the verifier permits: accept
getNumBodyUnitaries()==0 (treat as no-op/empty modifier) and >1 (handle by
iterating over body operations or by composing them appropriately during
lowering), and only fail for truly unsupported shapes; apply the same change to
the analogous check around the other occurrence (the block covering the lines
corresponding to 939-943) and update the notifyMatchFailure message to reflect
genuinely unsupported cases instead of rejecting valid empty/multi-body
modifiers.
---
Outside diff comments:
In `@mlir/lib/Conversion/QCToQIR/QCToQIR.cpp`:
- Around line 870-884: The code currently rejects nested CtrlOps; instead
preserve nested control contexts by saving and restoring the current control
state around the inline and erase steps: remove the notifyMatchFailure for
nested controls, store oldInCtrl = state.inCtrlOp and oldControls =
state.controls, then update state.inCtrlOp (e.g. state.inCtrlOp +=
op.getNumBodyUnitaries() or push the new value) and state.controls (append or
push adaptor.getControls()), call
rewriter.inlineBlockBefore(&op.getRegion().front(), op, adaptor.getTargets()),
then restore state.inCtrlOp = oldInCtrl and state.controls = oldControls before
returning and finally rewriter.eraseOp(op); this keeps nested control stacking
while inlining.
In `@mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp`:
- Around line 341-350: The rewrite currently erases an empty InvOp which drops
the mapping from its input qubits to its results; in
EraseEmptyInv::matchAndRewrite (for InvOp where op.getNumBodyUnitaries() == 0)
replace the erase with forwarding the op's qubit operands to its results using
the PatternRewriter replace API (e.g., call rewriter.replaceOp(op, /*forward op
qubit operands/results*/ op.getQubits() or the appropriate operand accessor)
instead of rewriter.eraseOp(op)) so any users of the InvOp results remain
connected.
In `@mlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cpp`:
- Around line 619-643: The code inserts controls into state.compoundControls for
compoundOp but never restores it, causing later operations to see stale
controls; fix by saving the original state.compoundControls (or record the
inserted controls) before the loop over compoundOp.getControls(), then after the
builder.ctrl lambda completes (after state.inCtrlOp = false and
state.targetArgs.clear()), restore state.compoundControls to the saved value (or
erase only the inserted controls) so compoundControls only reflects the current
CompoundOperation; reference state.compoundControls, compoundOp.getControls(),
builder.ctrl(..., [&](ValueRange){...}), state.inCtrlOp, and state.targetArgs to
locate where to save and restore.
---
Duplicate comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 959-964: The current remapping rebuilds state.targetsIn by
iterating op.getBody()->getArguments(), which uses inner block-arg indices and
reorders targets incorrectly for operands like qco.inv(%b=%arg1, %a=%arg0);
change the remapping to iterate op.getOperands() instead and for each operand
locate the corresponding outer qubit (either by using operand.getArgNumber() if
it's a BlockArgument or by matching the operand Value against entries in
outerQubits) and push that outer qubit into innerQubits, then assign
state.targetsIn = std::move(innerQubits); update the code around
state.targetsIn, innerQubits, op.getBody()->getArguments() to use
op.getOperands() lookup so the mapping follows the op operand order.
🪄 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: 89a450f0-9bcb-41ca-bb47-2c3849147370
📒 Files selected for processing (20)
mlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQIR/QCToQIR.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.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/quantum_computation_programs.cppmlir/unittests/programs/quantum_computation_programs.h
Comment on lines
+883
to
+887
| if (op.getNumBodyUnitaries() != 1) { | ||
| return rewriter.notifyMatchFailure( | ||
| op, | ||
| "Control modifiers with multiple body unitaries are not supported."); | ||
| } |
Contributor
There was a problem hiding this comment.
QCO→jeff still rejects the newly legal modifier bodies.
CtrlOp/InvOp verification now accepts bodies that are empty or contain multiple operations, but these two patterns still fail unless getNumBodyUnitaries() == 1. That leaves valid QCO IR unlowerable in this pass, including the empty-body cases this PR explicitly makes reachable after folding/canonicalization.
Also applies to: 939-943
🤖 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/QCOToJeff/QCOToJeff.cpp` around lines 883 - 887, The pass
currently rejects CtrlOp/InvOp when op.getNumBodyUnitaries() != 1; change the
guard in the QCO→jeff lowering (the check using op.getNumBodyUnitaries() and the
rewriter.notifyMatchFailure call) to allow zero or multiple-body cases that the
verifier permits: accept getNumBodyUnitaries()==0 (treat as no-op/empty
modifier) and >1 (handle by iterating over body operations or by composing them
appropriately during lowering), and only fail for truly unsupported shapes;
apply the same change to the analogous check around the other occurrence (the
block covering the lines corresponding to 939-943) and update the
notifyMatchFailure message to reflect genuinely unsupported cases instead of
rejecting valid empty/multi-body modifiers.
Member
Author
There was a problem hiding this comment.
Yeah, this will be added in a follow-up. We plan to add a pass that unrolls qco::CtrlOps. Once unrolled, it will be much easier to convert to jeff because jeff does not have the concepts of modifiers. jeff represents controlled gates via operation attributes.
Contributor
There was a problem hiding this comment.
@denialhaag, that makes sense — since jeff encodes controlled gates as operation attributes rather than region-based modifier ops, inlining/unrolling qco::CtrlOps beforehand is indeed the cleaner path. The guard keeping getNumBodyUnitaries() != 1 as a hard failure is a reasonable interim contract that prevents the pass from silently miscompiling not-yet-unrolled IR.
Would you like me to open a follow-up GitHub issue to track adding the qco::CtrlOp unrolling pass (and the corresponding relaxation of these guards in QCOToJeff)?
✏️ Learnings added
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1751
File: mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp:883-887
Timestamp: 2026-06-02T01:35:41.504Z
Learning: In munich-quantum-toolkit/core, the QCO→jeff lowering pass (`mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`) intentionally rejects `qco::CtrlOp` and `qco::InvOp` operations with zero or multiple body unitaries (guard: `getNumBodyUnitaries() != 1`). The jeff dialect represents controlled gates via operation attributes rather than region-based modifier ops, so the planned approach is to add a separate QCO pass that unrolls `qco::CtrlOp`s before the QCO→jeff conversion, at which point these guards can be relaxed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1574
File: mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp:18-18
Timestamp: 2026-03-20T13:14:32.940Z
Learning: When reviewing C++ code that uses LLVM's DenseMap/DenseSet with `llvm::SmallVector<T, N>`, do not flag `DenseMapInfo<llvm::SmallVector<T, N>>` as missing. LLVM's `llvm/ADT/SmallVector.h` already provides a full `DenseMapInfo<SmallVector<T, N>>` specialization (including empty/tombstone keys, hashing, and equality), so `DenseMapInfo<SmallVector<T, N>>` is a valid key-info trait in this context.
Learnt from: simon1hofmann
Repo: munich-quantum-toolkit/core PR: 1569
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1152-1158
Timestamp: 2026-03-31T15:21:47.577Z
Learning: When reviewing conversion/type-conversion code between the QC and QCO dialects (e.g., in MLIR conversion passes), do not add, reintroduce, or preserve an `isStatic`-style type-level flag on `qc::QubitType`/`qco::QubitType` (such as via an `!qc.qubit<static>` / type parameter). Staticness is tracked only at the operation level using `qc.static` / `qco.static` ops; type-conversion logic like `QCToQCOTypeConverter` should not encode staticness in qubit type parameters or any equivalent `QubitType` flag.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Support/Passes.cpp:26-73
Timestamp: 2026-04-07T10:16:39.619Z
Learning: In Munich-MLIR C++ code under `mlir/lib/`, follow the project convention of placing Doxygen comments on function declarations in the corresponding header files under `mlir/include/` (not on the function definitions in `.cpp`). During review, do not flag missing Doxygen comments on function definitions in a `.cpp` when the same function is documented on its declaration in the matching header.
Learnt from: J4MMlE
Repo: munich-quantum-toolkit/core PR: 1603
File: mlir/unittests/programs/qc_programs.cpp:311-315
Timestamp: 2026-04-12T10:44:13.813Z
Learning: When reviewing QC/QCO MLIR canonicalization for Pauli/SX gates in the munich-quantum-toolkit/core project, apply the pow(r) convention from discussion `#1225` exactly (including the global-phase sign):
- pow(r) X → gphase(-r·π/2); rx(r·π)
- pow(r) Y → gphase(-r·π/2); ry(r·π)
- pow(r) SX → gphase(-r·π/4); rx(r·π/2)
- pow(r) SXdg → gphase(+r·π/4); rx(-r·π/2)
Specifically, the global-phase coefficient must be NEGATIVE for X, Y, and SX, and POSITIVE for SXdg—do not suggest flipping these signs.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1620
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:288-299
Timestamp: 2026-04-13T22:19:35.390Z
Learning: In this repository, debug builds for MLIR conversion patterns are covered by CI, so using `assert` as an invariant guard inside conversion-pattern matching/arity checks is acceptable when the invariant should only fail due to `MQT_GATE_TABLE` / gate-mapping mismatches. When reviewing code in `mlir/lib/Conversion/**/*.cpp`, do not treat such debug-only `assert` failures as bugs that must trigger `notifyMatchFailure`; reserve `notifyMatchFailure` for true non-matching/normal control-flow cases rather than invariants protected by the gate-table mismatch assumptions.
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1638
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1200-1203
Timestamp: 2026-04-29T15:05:10.218Z
Learning: When writing MLIR conversion passes that use `ConversionPatternRewriter`, it is safe to replace terminator operations using the conversion-rewriter idiom `replaceOpWithNewOp<SomeTerminatorOp>(op, ...)`. `ConversionPatternRewriter` defers rewriting: it schedules the old op for erasure (unlinked) and commits only after the deferred rewrite is processed/verified, so you should not see transient IR states with two terminators in the same block. Only use a manual `setInsertionPoint(op); create; eraseOp(op)` sequence when using a plain `PatternRewriter` (non-conversion), where rewrites apply immediately to the live IR and transient block state could be visible.
Member
Author
There was a problem hiding this comment.
No, I'll open the issue myself later today. 🙂
This was referenced Jun 2, 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
1 participant
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 enforce that their bodies contain a single unitary operation followed by a
yieldoperation.The PR isn't exactly ready yet, but I wanted to open it anyway since the tests are finally passing again. Things that are still missing:
QuantumComputationto QCjeffround-trip at the moment; maybe a follow-up)getUnitaryMatrix()methods to support bodies with multiple unitary operations (see #1760)Fixes #1678
Fixes #1727
Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).