✨ Add power modifier to MLIR#1603
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
burgholzer
added
feature
J4MMlE
force-pushed
the
pow-modifier
branch
2 times, most recently
from
f017b29 to
4964ed0
Compare
|
|
||
| // Move supporting ops (constants, arithmetic) out of the body so their | ||
| // Values are accessible from outside and survive PowOp erasure. | ||
| for (auto& bodyOp : llvm::make_early_inc_range(*op.getBody())) { |
There was a problem hiding this comment.
Because inv creates a negf operation inside its body we must move out these operations when we want to apply gate specific canonicalizations. Currently I do this here unconditionally - which feels pretty wrong. Isnt there a way for hoisting up operations which can/should be moved out of the body? Should maybe inv and pow run such a hositing up thing?
There was a problem hiding this comment.
I am pretty sure that these operations are automatically hoisted up as part of canonicalization.
I remember this being problematic for nested Control and Inverse modifiers already. But we never had to handle that explicitly.
There was a problem hiding this comment.
Code for inv { pow (2) { rx(0.123) }}:
module {
func.func @testPow() {
%q0_0 = qco.alloc : !qco.qubit
%q0_1 = qco.inv (%a = %q0_0){
%a_1 = qco.pow (2.000000e+00) (%b = %a) {
%c_0 = arith.constant 0.123 : f64
%b_1 = qco.rx(%c_0) %b : !qco.qubit -> !qco.qubit
qco.yield %b_1
} : {!qco.qubit} -> {!qco.qubit}
qco.yield %a_1
} : {!qco.qubit} -> {!qco.qubit}
qco.sink %q0_1 : !qco.qubit
return
}
}will lead to this state (swap pow and inv and then do ReplaceWithKnownGates for inv and rx:
module {
func.func @testPow() {
%cst = arith.constant 1.230000e-01 : f64
%0 = qco.alloc : !qco.qubit
%1 = qco.pow(2.000000e+00) (%arg0 = %0) {
%2 = arith.negf %cst : f64
%3 = qco.rx(%2) %arg0 : !qco.qubit -> !qco.qubit
qco.yield %3
} : {!qco.qubit} -> {!qco.qubit}
qco.sink %1 : !qco.qubit
return
}
}If %2 is not explicitly moved out of the pow modifier it is not possible to merge pow and rx with the known canonicalization. So leaving the code out leads to segfault (ask me how I know lol).
There was a problem hiding this comment.
So the segfault possibly occurs, because we want to create pow(r) { rx(θ) } → rx(r*θ) with r = %2. %2 only exists inside the body of pow which gets replaced/deleted when doing replaceOpWithNewOp, creating a dangling reference to %2.
There was a problem hiding this comment.
Hm. This very much depends on the sequence of the individual canonicalization passes.
Because there definitely is a canonicalization that will pull out the %2 = arith.negf %cst : f64 and fold it into the %cst = arith.constant 1.230000e-01 : f64 statement, which would result in the elimination of the arith.negf instruction and would also eliminate the segfault cause.
There has to be a way to address this. Either by making the canonicalization more robust (e.g., by moving the body of the original pow operation to the outside and then performing the manipulation) or by marking the canonicalization only as valid whenever the body of the operation only consists of the UnitaryOperation (and the yield).
The first solution seems to be more desirable to me.
This is kind of like inlining plus some additional manipulation.
I am pretty sure https://mlir.llvm.org/doxygen/classmlir_1_1RewriterBase.html#a4307b18bc25cec55349236d556cc89bd should do the trick here.
The canonicalization is basically inlining the block and adding a arith.mulf (or whatever the instruction is called) on the rotation angle argument before replacing the rotation angle argument with the new rotation angle.
|
Ok, I discovered another problem, consider this example:
My idea then: the then and then The only thing im not to sure about, whether the Concrete implementation: instead of emiting 2 operations, the canonicalization creates an inner operation and a preamble.
I dont know if creating a whole interface for that is premature, i believe only the power modifier emits 2-operation-canonicalizations so the whole thing could stay within power modifier. edit: maybe, when keeping it general it would make sense having this as an interface, so that other operations could be moved out as well (like arithmetic operations which e.g. |
J4MMlE
force-pushed
the
pow-modifier
branch
2 times, most recently
from
2375c9d to
993a3eb
Compare
J4MMlE
force-pushed
the
pow-modifier
branch
5 times, most recently
from
faeba24 to
05c9335
Compare
|
While implementing the QC<->QCO conversion tests, I noticed some friction: |
J4MMlE
force-pushed
the
pow-modifier
branch
2 times, most recently
from
91700af to
b1759c7
Compare
|
Ok, commits cleaned up, linting and coverage succeeds - i think this is ready for a first look @DRovara @burgholzer |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a PowOp modifier across QC and QCO: TableGen op declarations, builder APIs, bidirectional conversions, extensive canonicalization/folding and unitary-matrix support, math helpers, and broad unit-test/program-builder coverage for pow semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QCBuilder as QC ProgramBuilder
participant QCPow as QC PowOp
participant Converter as QC↔QCO Converter
participant QCOBuilder as QCO ProgramBuilder
participant Canon as Canonicalizer
User->>QCBuilder: pow(exponent, body)
QCBuilder->>QCPow: create PowOp (exponent, region from body)
QCPow->>Converter: convert (QC→QCO) / request mapping
Converter->>QCOBuilder: create qco.pow (exponent), clone region
QCOBuilder->>QCOBuilder: build region, create block args, yield
QCOBuilder->>Converter: return qco.pow
QCPow->>Canon: register for canonicalization
Canon->>Canon: apply folding rules (scale params, inline/erase, merge)
alt folded
Canon->>User: replace with simplified ops
else unchanged
Canon->>User: leave PowOp
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp (1)
68-76:⚠️ Potential issue | 🔴 CriticalThe rewrite set is broader than the body invariant it actually preserves.
verify()admits helper ops before the body unitary, butNegPowToInvPowandMoveCtrlOutsideclone only the unitary,MergeNestedPowonly retargets the moved op’s qubit operands, and the identity-producing branches replacepowwith plain pass-through values. With body-localarith.*or captured values, that can dangle defs after replacement; with a nested parent modifier, it can leave the parent region with no unitary at all. Either tightenverify()back to the single-unitary form, or preserve/hoist the full prologue and rewrite enclosing modifiers in the same pattern.Also applies to: 158-169, 183-199, 218-221, 244-259, 284-289, 482-499, 596-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp` around lines 68 - 76, The rewrite patterns (NegPowToInvPow, MoveCtrlOutside, MergeNestedPow, and functions like tryReplaceWithNamedPhaseGate operating on PowOp) assume a single-unitary body but verify() currently allows helper ops before the unitary; this leads to dangling defs or removal of the unitary while leaving prologue helpers behind. Fix by either tightening verify() on PowOp to require exactly one unitary op in the body (no prologue helpers), or update each rewrite (NegPowToInvPow, MoveCtrlOutside, MergeNestedPow and tryReplaceWithNamedPhaseGate) to preserve/hoist the full prologue: when cloning or retargeting the unitary, also clone or move any preceding helper ops and update their uses, ensure replacements replace the entire region (including helpers) or remap all results/operands so no defs are left dangling, and update operand/region remapping logic to maintain the same external SSA edges for PowOp.mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp (1)
60-68:⚠️ Potential issue | 🔴 CriticalThese rewrites assume a stricter body invariant than
verify()enforces.
verify()currently permits arbitrary non-unitary ops before the body unitary, but the structural rewrites only preservegetBodyUnitary(), and some folds erasePowOpoutright. With body-localarith.*defs, that can dangle operands after replacement; with a nested parent modifier, it can leave the parent body with no unitary at all. Either tighten the body back tounitary + yield, or hoist/inline the full prologue and rewrite the enclosing modifier atomically.Based on learnings,
InvOp::verifyintentionally restricts the body region to exactly two operations (one unitary + yield).Also applies to: 149-158, 170-173, 188-190, 204-207, 230-235, 431-450, 498-517
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 1039-1047: The target/control accessors for the pow op are
inconsistent: getNumTargets() and getTarget(size_t) use overall qubit semantics
while getTargets() delegates to getBodyUnitary(), causing mismatched behavior;
update the pow op's accessors to mirror the InvOp pattern by delegating
target/control-related calls to getBodyUnitary() instead of using
getNumQubits()/getQubit(), i.e., make getNumTargets() call
getBodyUnitary().getNumTargets(), make getTarget(size_t i) call
getBodyUnitary().getTarget(i), and ensure
getNumControls()/getControls()/getControl() follow the same body-unitary
delegation so all UnitaryInterface accessors are consistent with
getBodyUnitary().
In `@mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp`:
- Around line 71-75: The current replacement only clones
innerPow.getBodyUnitary().getOperation(), losing any non-terminator preamble ops
(constants/arithmetic) that feed the unitary; update the replacement passed to
rewriter.replaceOpWithNewOp<PowOp> / InvOp::create to iterate over
innerPow.getBody() and clone every non-terminator operation (preserving original
order) into the new region instead of cloning only innerPow.getBodyUnitary(), so
values used by the unitary remain available after MovePowOutside transforms.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 173-201: The NegPowToInvPow rewrite flips sign of the exponent by
replacing pow(p){...} with pow(-p){inv{...}}, which yields conjugated
eigenphases for operators that have eigenvalue -1 and thus produces incorrect
results; modify NegPowToInvPow (the matchAndRewrite for PowOp using
getExponentValue and creating an InvOp) to only apply when the operand unitary's
spectrum is guaranteed not to contain −1 (e.g., obtain the unitary via
getUnitaryMatrix() or a new op method and check no eigenvalue is within an
epsilon of -1), and otherwise skip the rewrite (return failure); alternatively
remove the NegPowToInvPow pattern entirely if no safe spectral test is
available.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp`:
- Around line 91-109: Add tests that exercise fractional powers of RXX to hit
the eigendecomposition/branch logic: create additional test cases in
QCOMatrixTest (e.g., new TEST_F methods or extend PowRxxOpMatrix) that build a
module with powRxx at fractional exponents like 0.5 and -0.5 (use the same
QCOProgramBuilder entry used for powRxx but with the power changed), retrieve
the PowOp via func::FuncOp and PowOp/getUnitaryMatrix(), compute the expected
matrix via dd::opToTwoQubitGateMatrix(qc::OpType::RXX, {theta * power}) into an
Eigen::Matrix4cd, and assert matrix->isApprox(eigenDefinition) for both 0.5 and
-0.5 to validate principal-branch/eigendecomposition behavior.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 893-894: The test case name string in the QCOTestCase entry is
incorrect: change QCOTestCase{"TdgThenS", MQT_NAMED_BUILDER(tdgThenT),
MQT_NAMED_BUILDER(emptyQCO)} so the human-readable name matches the builder by
renaming "TdgThenS" to "TdgThenT" (to align with the tdgThenT builder and
emptyQCO), ensuring test failure messages are not misleading.
In `@mlir/unittests/programs/qc_programs.cpp`:
- Around line 311-315: The global-phase sign is inverted in the reference
decompositions: in powThirdXRef change the gphase argument from -1.0/3.0 *
std::numbers::pi / 2.0 to +1.0/3.0 * std::numbers::pi / 2.0 so the reference
circuit matches the U/R* convention (i.e., X = gphase(+π/2)·rx(π)); apply the
same flip of sign for the gphase calls in the other reference functions
mentioned (the blocks at 357-361, 733-737, 789-793) so they use positive phase
corrections consistent with GPhaseOp(inputPhase - outPhase).
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 68-76: The rewrite patterns (NegPowToInvPow, MoveCtrlOutside,
MergeNestedPow, and functions like tryReplaceWithNamedPhaseGate operating on
PowOp) assume a single-unitary body but verify() currently allows helper ops
before the unitary; this leads to dangling defs or removal of the unitary while
leaving prologue helpers behind. Fix by either tightening verify() on PowOp to
require exactly one unitary op in the body (no prologue helpers), or update each
rewrite (NegPowToInvPow, MoveCtrlOutside, MergeNestedPow and
tryReplaceWithNamedPhaseGate) to preserve/hoist the full prologue: when cloning
or retargeting the unitary, also clone or move any preceding helper ops and
update their uses, ensure replacements replace the entire region (including
helpers) or remap all results/operands so no defs are left dangling, and update
operand/region remapping logic to maintain the same external SSA edges for
PowOp.
🪄 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: e7ba28cd-bad0-4888-8e3a-03526f96e57b
📒 Files selected for processing (22)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
The problem with just invoking the conversion itself is that the verification becomes very hard at that point since canonicalization has not been run. Constants might be in different places. Some canonicalizations on classical constructs may not have been applied, etc. |
Hm. This one is tricky.
Technical nitpick, but these operations do not operate on the same qubit since the gphase operation has no target qubits.
This is definitely valid as it is also one of the existing canonicalizations for the CtrlOp when that op contains a GphaseOp. Before commenting on the proposal below, a couple of observations: --> --> --> At this stage, the real problem appears, because the The easy way out here is to not permit such PowOp rewrites in CtrlOp modifiers.
I believe I like the general direction of this, but I would not want to create that complexity as part of this PR. Let's rather get a working solution here for the majority of the cases and live with the fact that some constructs cannot be resolved at the moment, even though it would be technically possible. |
J4MMlE
force-pushed
the
pow-modifier
branch
2 times, most recently
from
1e9588b to
452c0fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp (1)
173-177:⚠️ Potential issue | 🔴 CriticalPreserve the full modifier body when rebuilding nested rewrites.
These patterns rebuild the replacement from
getBodyUnitary()only, butPowOp::verify()explicitly allows helper ops before that unitary. A case likepow(-2){ %t = arith.negf %c; rx(%t) }orpow(a){ pow(b){ rx(%t) } }will clonerxwithout cloning%t, then erase the original region and leave the new op with dangling operands.
inlineBlockBefore/full-region cloning is needed here, not just cloning the terminal unitary op.Also applies to: 191-193, 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp` around lines 173 - 177, The replacement currently clones only the terminal unitary via getBodyUnitary() inside InvOp::create when rebuilding PowOp (see PowOp, InvOp::create, and uses of rewriter.replaceOpWithNewOp), which drops helper ops declared earlier in the region and leaves dangling operands; instead inline the full region/block into the new op (use inlineBlockBefore or clone the entire region) so all helper ops and SSA values are preserved before erasing the original region — update the PowOp replacement sites (including the occurrences around getBodyUnitary() at the other noted locations) to copy the whole region/body rather than just cloning the terminal unitary.mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp (1)
246-261:⚠️ Potential issue | 🔴 Critical
MoveCtrlOutsidedrops helper ops from the controlled body.This rewrite only clones
innerCtrlOp.getBodyUnitary(). If the ctrl body contains%theta = arith.negf ...; qco.rx(%theta) ..., the new innerqco.powwill cloneqco.rxbut not%theta, and the old ctrl region is erased immediately afterward. The replacement then references values defined in a deleted region.Please inline or clone the full ctrl body (minus its yield), not just the terminal unitary op.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp` around lines 246 - 261, The replacement currently clones only innerCtrlOp.getBodyUnitary().getOperation(), which misses helper ops and leaves references to values from the erased original region; update the lambda passed to PowOp::create (the one capturing innerBody and using rewriter.clone) to clone the entire control region body (innerCtrlOp.getBody()) except its yield operation rather than only getBodyUnitary(), mapping each argument as you already do, then insert the cloned block body results as the pow's results; ensure you remove or skip cloning the yield op so the new cloned sequence contains all helper ops (e.g., any arith.negf) and their values are available to the newly created qco.pow instead of referencing the soon-to-be-deleted region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 1017-1031: The op description currently asserts "r < 0: equivalent
to inv @ pow(-r) @ g" as a general rule; update the text in the pow op
description (QCOps.td) to clarify that this equivalence only holds for integer
exponents and that the canonicalization NegPowToInvPow is guarded by
utils::isIntegerExponent(-exp); explicitly note that fractional negative
exponents can produce principal-branch phase conjugations (e.g., why the NegPowH
test keeps pow(-0.5){H} unchanged) so the identity is not universally valid for
non-integer r.
In `@mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp`:
- Around line 58-80: The rewrite MovePowOutside currently assumes (U^p)† =
(U†)^p for any exponent; guard the pattern by checking the exponent extracted
from innerPow (via PowOp::getExponentValue or similar) is an integer before
performing the replacement and return failure for non-integer/fractional
exponents; locate the check in MovePowOutside::matchAndRewrite (where innerPow
and exponent are read) and add an integer-test (e.g., exponent ==
std::floor(exponent) or equivalent safe integer predicate) so only integer
exponents trigger rewriter.replaceOpWithNewOp; leave all other behavior
unchanged.
In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 233-245: The current guard that prevents X/Y/SX/SXdg folds inside
modifiers must also cover folds that produce an identity (no-op) and would erase
the sole body unitary; update the folding logic in PowOp.cpp so that before
performing any identity-producing rewrite (examples: pow(0) removal, zero-angle
phase normalization, or even-parity HOp/ECROp/SWAPOp folds that delete the
body), you check the parent op via op->getParentOp() and return failure if the
parent is a modifier (llvm::isa<CtrlOp, InvOp, PowOp>). Apply this same
parent-modifier gate where utils::isIntegerExponent is checked and where the
code performs in-place erasure of the child, or alternatively implement the
rewrite by reconstructing the parent modifier (CtrlOp/InvOp/PowOp) instead of
erasing the child in place.
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 813-815: Replace the unsafe iterator pair used in the loop that
zips innerTargetsOut and targetsOut with llvm::zip_equal to enforce equal-length
invariants: locate the for-loop in QCOProgramBuilder.cpp that currently reads
"for (const auto& [target, targetOut] : llvm::zip(innerTargetsOut, targetsOut))
{ updateQubitTracking(target, targetOut); }" and change llvm::zip to
llvm::zip_equal so mismatched lengths will assert rather than silently truncate;
keep the updateQubitTracking call and adjust any necessary includes if zip_equal
requires them.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp`:
- Around line 84-118: The rewrite in MovePowOutside (matchAndRewrite on InvOp)
is unsafe for non-integer exponents; add the same integer-exponent guard used by
the negative-pow canonicalization: after retrieving innerPow and exponent
(innerPow.getExponentValue()), check that the exponent is an integer (e.g.,
exponent == floor(exponent) or call an existing PowOp helper like
isIntegerExponent()) and return failure if it is not integer, so the pattern
only rewrites when the exponent is integral.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 286-304: The current guard in the PowOp folding logic only blocks
rewrites that emit a GPhase plus gate when nested in a modifier (the
llvm::isa<XOp, YOp, SXOp, SXdgOp> && llvm::isa<CtrlOp, InvOp,
PowOp>(op->getParentOp()) check), but identity-producing folds (pow(0),
zero-angle phase normalization, and integer-exponent cases that remove
H/ECROp/SWAPOp/body) can still erase the sole body and leave an invalid
modifier; update the folding pre-checks to detect these identity-producing
outcomes and also return failure when op->getParentOp() is a CtrlOp, InvOp, or
PowOp: specifically extend the existing nesting guard to cover cases where r==0
or phase-normalization yields zero phase and the integer-exponent paths
(utilizing utils::isIntegerExponent and the same innerOp checks) would eliminate
the innerOp, or alternatively implement the fold to rewrite the parent modifier
(op->getParentOp()) in one step rather than erasing the child in place so nested
modifiers are never left with only qco.yield.
---
Duplicate comments:
In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 173-177: The replacement currently clones only the terminal
unitary via getBodyUnitary() inside InvOp::create when rebuilding PowOp (see
PowOp, InvOp::create, and uses of rewriter.replaceOpWithNewOp), which drops
helper ops declared earlier in the region and leaves dangling operands; instead
inline the full region/block into the new op (use inlineBlockBefore or clone the
entire region) so all helper ops and SSA values are preserved before erasing the
original region — update the PowOp replacement sites (including the occurrences
around getBodyUnitary() at the other noted locations) to copy the whole
region/body rather than just cloning the terminal unitary.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 246-261: The replacement currently clones only
innerCtrlOp.getBodyUnitary().getOperation(), which misses helper ops and leaves
references to values from the erased original region; update the lambda passed
to PowOp::create (the one capturing innerBody and using rewriter.clone) to clone
the entire control region body (innerCtrlOp.getBody()) except its yield
operation rather than only getBodyUnitary(), mapping each argument as you
already do, then insert the cloned block body results as the pow's results;
ensure you remove or skip cloning the yield op so the new cloned sequence
contains all helper ops (e.g., any arith.negf) and their values are available to
the newly created qco.pow instead of referencing the soon-to-be-deleted region.
🪄 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: 884b521e-f366-4316-bcaa-165d85c79c10
📒 Files selected for processing (22)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 275-278: Remove the stale commented-out insertion-point call "//
rewriter.setInsertionPoint(op);" from PowOp.cpp near the PowOp lowering logic
and keep only the explanatory comment if needed; locate the block that currently
says "Set insertion point before the PowOp so that new ops (constants,
arithmetic) are created outside the body region." and delete the dead code line
so the comment no longer contains abandoned code (ensure you do not change the
surrounding logic that relies on the rewriter's default insertion point or
moveOpBefore calls).
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 305-310: The parity checks currently cast the exponent (r) to
int64_t (e.g., static_cast<int64_t>(r) % 2 == 0), which can UB for huge
integrals; replace those casts with a floating-point parity test using std::fmod
on the exponent (e.g., std::fmod(std::fabs(r), 2.0) == 0.0) after you still
ensure utils::isIntegerExponent(r) holds, and update all five sites that perform
the parity check (the pre-check with insideModifier + ECROp/SWAPOp, the HOp
cases, and the ECROp/SWAPOp cases in both the QCO and QC PowOp implementations)
so they use std::fmod/std::fabs on the F64Attr-derived double instead of
narrowing to int64_t.
🪄 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: 3854a79b-7622-44eb-817c-e51d25348ab6
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp (1)
174-225:⚠️ Potential issue | 🔴 CriticalPreserve the full body in these modifier rewrites.
NegPowToInvPow,MergeNestedPow, andMoveCtrlOutsiderebuild the replacement op by cloning onlygetBodyUnitary(). That is no longer safe here: the same file already handles legal body preamble ops (arith.constant,arith.negf, etc.) inFoldPowIntoGate, so these three patterns can drop values the cloned unitary still depends on and recreate the dangling-use/segfault class we already hit in this PR. Please inline or clone every non-terminator op from the source body into the new region, not just the unitary op.#!/bin/bash set -euo pipefail echo "Clone-only rewrites in QC PowOp canonicalizations:" sed -n '174,225p' mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp echo echo "The same file already assumes legal non-unitary preamble ops can exist in pow bodies:" sed -n '266,272p' mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppBased on learnings: modifier bodies in this area may legally contain constants and arithmetic before the unitary, so rewrites must preserve the full non-terminator body rather than only
getBodyUnitary().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp` around lines 174 - 225, The three rewrites (NegPowToInvPow, MergeNestedPow, MoveCtrlOutside) only clone getBodyUnitary() into the new region which drops any non-terminator preamble ops (constants, arith.*) and causes dangling uses; locate the replaceOpWithNewOp/Op::create sites in these structs and change the body cloning to copy/inline the entire source region body: iterate the original PowOp body region and clone every non-terminator op (not just the unitary op) into the new region before cloning the terminal unitary, preserving operands and mappings (use the PatternRewriter region/Block cloning utilities or manually clone ops with value remapping) so all preamble values remain available to the cloned unitary.
🤖 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/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 231-234: The mutations to innerPow (after rewriter.moveOpBefore)
bypass PatternRewriter notifications, so wrap the in-place changes in
rewriter.modifyOpInPlace(innerPow, [&](){
innerPow->setOperands(op.getInputQubits());
innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); }); to ensure the
pattern driver re-enqueues the op (so ErasePow0 can fire when merged == 0); keep
the moveOpBefore and final rewriter.replaceOp(op, innerPow->getResults()) as-is.
---
Duplicate comments:
In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 174-225: The three rewrites (NegPowToInvPow, MergeNestedPow,
MoveCtrlOutside) only clone getBodyUnitary() into the new region which drops any
non-terminator preamble ops (constants, arith.*) and causes dangling uses;
locate the replaceOpWithNewOp/Op::create sites in these structs and change the
body cloning to copy/inline the entire source region body: iterate the original
PowOp body region and clone every non-terminator op (not just the unitary op)
into the new region before cloning the terminal unitary, preserving operands and
mappings (use the PatternRewriter region/Block cloning utilities or manually
clone ops with value remapping) so all preamble values remain available to the
cloned unitary.
🪄 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: 6fc1a075-e648-4f9a-8507-763ee3b465db
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp
| rewriter.moveOpBefore(innerPow, op); | ||
| innerPow->setOperands(op.getInputQubits()); | ||
| innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); | ||
| rewriter.replaceOp(op, innerPow->getResults()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the codebase consistently routes in-place mutations through
# rewriter.modifyOpInPlace in similar canonicalization patterns.
rg -n -C2 'modifyOpInPlace' mlir/lib/Dialect | head -60
echo '---'
rg -nP '\->\s*setOperands\(' mlir/lib/Dialect | head -40Repository: munich-quantum-toolkit/core
Length of output: 1435
🏁 Script executed:
#!/bin/bash
# 1. Check if similar merge/canonicalization patterns in InvOp and CtrlOp use direct mutations
rg -A5 'moveOpBefore' mlir/lib/Dialect/QCO/IR/Modifiers/ | grep -E '(moveOpBefore|setOperands|modifyOpInPlace|replaceOp)'
echo '---'
# 2. Look at the full MergeNestedPow pattern context to understand if merged==0 case is a concern
sed -n '210,240p' mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppRepository: munich-quantum-toolkit/core
Length of output: 2088
Wrap in-place mutations in rewriter.modifyOpInPlace to notify the pattern driver.
The moveOpBefore already goes through the rewriter, but the subsequent setOperands and setExponentAttr calls bypass PatternRewriter notifications. When merged == 0, the modified innerPow should be re-canonicalized by the ErasePow0 pattern, but direct mutations may prevent pattern-driver re-enqueuing. Wrap the edits in rewriter.modifyOpInPlace(...) to ensure the merged op is reconsidered.
Note: Similar patterns in mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp and CtrlOp.cpp use the same direct-mutation style, so this is a codebase-wide improvement opportunity.
♻️ Proposed refactor
- rewriter.moveOpBefore(innerPow, op);
- innerPow->setOperands(op.getInputQubits());
- innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged));
- rewriter.replaceOp(op, innerPow->getResults());
+ rewriter.moveOpBefore(innerPow, op);
+ rewriter.modifyOpInPlace(innerPow, [&]() {
+ innerPow->setOperands(op.getInputQubits());
+ innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged));
+ });
+ rewriter.replaceOp(op, innerPow->getResults());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp` around lines 231 - 234, The
mutations to innerPow (after rewriter.moveOpBefore) bypass PatternRewriter
notifications, so wrap the in-place changes in
rewriter.modifyOpInPlace(innerPow, [&](){
innerPow->setOperands(op.getInputQubits());
innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); }); to ensure the
pattern driver re-enqueues the op (so ErasePow0 can fire when merged == 0); keep
the moveOpBefore and final rewriter.replaceOp(op, innerPow->getResults()) as-is.
There was a problem hiding this comment.
@J4MMlE I believe @coderabbitai might have a point here.
There was a problem hiding this comment.
@burgholzer, @J4MMlE — glad this is useful. To confirm the correctness concern with official backing:
All IR mutations, including creations, are required to be performed via the PatternRewriter class, because the underlying pattern driver may have state that would be invalidated when a mutation takes place.
The greedy pattern driver specifically listens to notifications to populate the worklist, so direct calls to setOperands and setExponentAttr that bypass the rewriter mean the driver won't know to re-enqueue innerPow for further rewrites. This is particularly relevant for the merged == 0.0 case, where ErasePow0 should fire on the mutated innerPow but won't unless the driver is notified.
A convenience wrapper modifyOpInPlace is provided that wraps a startOpModification and finalizeOpModification around a callback.
The proposed fix remains:
- rewriter.moveOpBefore(innerPow, op);
- innerPow->setOperands(op.getInputQubits());
- innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged));
- rewriter.replaceOp(op, innerPow->getResults());
+ rewriter.moveOpBefore(innerPow, op);
+ rewriter.modifyOpInPlace(innerPow, [&]() {
+ innerPow->setOperands(op.getInputQubits());
+ innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged));
+ });
+ rewriter.replaceOp(op, innerPow->getResults());The same fix should also be applied to any analogous direct-mutation patterns in InvOp.cpp and CtrlOp.cpp across both the QC and QCO dialects.
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1603
File: mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp:173-201
Timestamp: 2026-04-16T12:22:48.409Z
Learning: For QCO power/canonicalization rewrite patterns that transform a negative exponent into an inverse-power form (e.g., rewriting pow(p){U} (with p<0) into pow(-p){inv{U}}), ensure the identity is mathematically valid for the exponent type. This rewrite is only correct when the exponent is an integer; for fractional exponents the relation U^{-r}=(U^{-1})^r can fail due to principal-branch behavior of complex logs (e.g., for eigenvalue -1, Hermitian/self-inverse gates like H/X/Y/Z/ECR/SWAP). Guard the canonicalization with a check such as `utils::isIntegerExponent(-exp)` (or equivalent) so the pattern only fires for negative integer exponents.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1603
File: mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp:84-118
Timestamp: 2026-04-18T08:51:01.413Z
Learning: In `mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp` and `mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp`, the `MovePowOutside` canonicalization pattern that rewrites `inv(pow(p){U})` must use the identity `(U^p)^{-1} = U^{-p}` (negate the exponent, keep body unchanged), NOT `(U^p)^{-1} = (U^{-1})^p` (keep exponent, wrap body in InvOp). The latter identity only holds for integer `p` due to principal-branch complex-log issues for eigenvalue -1. The agreed approach (burgholzer, PR `#1603`): emit `pow(-p){U}` unconditionally; for negative integer results, the existing `NegPowToInvPow` pattern then fires to produce `pow(|p|){inv{U}}`; negative fractional exponents are left as `pow(-r){U}` and do not require further reduction.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:242-250
Timestamp: 2026-03-30T18:16:05.889Z
Learning: In `mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` (PR `#1580`), stateful conversion patterns (`ConvertMemRefLoadOp`, `ConvertQCResetOp`, `ConvertQCBarrierOp`, etc.) intentionally use `rewriter.eraseOp(op)` instead of `rewriter.replaceOp(op, ...)` when the source op and target op have different result counts/types. Instead of using the conversion framework's value replacement, they store result mappings in `LoweringState` (qubitMap, qtensorMap, qubitInfos) and downstream patterns read from that state. Do not flag `eraseOp` without `replaceOp` as a bug in these stateful patterns — the state map is the correct mechanism for tracking value flow.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1407
File: mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp:0-0
Timestamp: 2026-04-02T08:36:19.147Z
Learning: In `mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`, the `MergeRotationGatesPattern::matchAndRewrite` must unconditionally emit a `GPhaseOp` to correct the intrinsic global phase of the synthesized `UOp`. Because `U(θ,ϕ,λ) = exp(i(ϕ+λ)/2) * RZ(ϕ)RY(θ)RZ(λ)` in both Qiskit and MQT conventions, every emitted `UOp` carries phase `(φ+λ)/2` relative to the pure SU(2) rotation. The correction must always be `GPhaseOp(inputPhase - outPhase)` where `inputPhase` defaults to `constants.zero` for pure SU(2) input chains (RX, RY, RZ, ROp) and `outPhase = (phi + lambda) / 2`. Conditionally skipping the `GPhaseOp` when `phaseAccum` is `nullopt` is a bug that leaves spurious phase on pure SU(2) chains.
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: simon1hofmann
Repo: munich-quantum-toolkit/core PR: 1665
File: mlir/lib/Dialect/QCO/Transforms/NativeSynthesis/SingleQubit.cpp:360-380
Timestamp: 2026-04-27T13:44:30.955Z
Learning: In `mlir/lib/Dialect/QCO/Transforms/NativeSynthesis/SingleQubit.cpp` (PR `#1665`), `SingleQubitMode::U3` matrix synthesis uses `EulerDecomposition::anglesFromUnitary(m, EulerBasis::ZYZ)` which returns `{theta, phi, lambda, phase}` already convention-matched to the `qco.u` (OpenQASM 2) gate. The returned `phase` already absorbs the U-gate's intrinsic `exp(i*(phi+lambda)/2)` offset, so `emitGPhaseIfNonTrivial(rewriter, loc, phase)` followed by `e.u(inQubit, angles[0], angles[1], angles[2])` exactly reconstructs the original unitary. Do NOT suggest subtracting `(angles[1]+angles[2])/2` from the emitted global phase — it would over-correct.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1479
File: mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp:1367-1370
Timestamp: 2026-03-09T02:58:00.657Z
Learning: Guideline: In MLIR conversion code, using rewriter.replaceOpWithNewOp<OtherOp>(oldOp, ...) inserts the new operation before oldOp. For terminator replacements needing a specific end-of-block position, use the idiom: rewriter.setInsertionPointToEnd(block); OtherOp::create(rewriter, loc, ...); rewriter.eraseOp(oldOp). Alternatively, avoid transient two-terminator state by using: rewriter.setInsertionPoint(oldOp); OtherOp::create(rewriter, loc, ...); rewriter.eraseOp(oldOp). This applies broadly to MLIR conversion passes and should be checked for correct insertion behavior when rewriting ops.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Dialect/QTensor/IR/Operations/ExtractOp.cpp:85-107
Timestamp: 2026-04-07T12:46:13.250Z
Learning: In `mlir/lib/Dialect/QTensor/IR/Operations/ExtractOp.cpp`, the `RemoveInsertExtractPair` canonicalization pattern's `matchAndRewrite` uses `getConstantIntValue` guards before commuting `ExtractOp` past any `InsertOp` or nested `ExtractOp` in the tensor chain. If either the traversed op's index or the target extract's index is not a compile-time constant, the pattern returns `failure()` to avoid unsound commutation with potentially aliasing dynamic indices. This guard was added proactively (PR `#1580`) to ensure correctness once structured control flow (SCF) with dynamic loop indices is supported in the pipeline.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:203-204
Timestamp: 2026-03-30T20:19:39.044Z
Learning: In `mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` (PR `#1580`), the intended approach for enforcing that only `memref.alloc`, `memref.load`, and `memref.dealloc` are supported for qubit-bearing memrefs is to use `target.addDynamicallyLegalDialect<memref::MemRefDialect>()` with a predicate that marks any memref op illegal when it touches a `memref<...x!qc.qubit>` type. This replaces the three individual `addDynamicallyLegalOp` calls and ensures unsupported qubit-bearing memref ops (e.g. `memref.cast`) produce a clear conversion error rather than triggering an assert inside the pattern.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1620
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:288-299
Timestamp: 2026-04-13T22:19:38.818Z
Learning: In `munich-quantum-toolkit/core`, debug builds are regularly tested in CI. Therefore, `assert` statements in MLIR conversion patterns (e.g., arity checks in `ConvertQCOGateToQC::matchAndRewrite` in `mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`) are an acceptable guard for invariants that would only drift due to `MQT_GATE_TABLE` mismatches. Do not flag debug-only asserts in conversion pattern arity checks as bugs requiring `notifyMatchFailure` — the CI debug build coverage is considered sufficient protection.
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1638
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1259-1267
Timestamp: 2026-04-14T14:57:56.777Z
Learning: In `mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` (PR `#1638`), the SCF conversion patterns `ConvertSCFForOp`, `ConvertSCFWhileOp`, and `ConvertSCFIfOp` assume that any SCF operation they convert carries exclusively qubit/qtensor values — never mixed qubit/classical. Classical-only SCF ops are kept legal by the `target.addDynamicallyLegalDialect<scf::SCFDialect>` predicate (which gates on the `needChange` attribute set by `collectQubitValuesInsideSCFOps`), so they are never routed to these patterns. Therefore, when the patterns rebuild the op from `qcoTargets`, all iter_args/results are guaranteed to be qubit-typed and nothing is silently dropped. Do not flag the absence of classical iter_arg threading as a bug in these patterns.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1569
File: mlir/include/mlir/Dialect/QC/IR/QCOps.td:0-0
Timestamp: 2026-04-01T13:22:25.156Z
Learning: In `mlir/include/mlir/Dialect/QC/IR/QCOps.td` (PR `#1569`), `qc::InvOp`'s accessor methods (`getNumTargets`, `getNumControls`, `getTarget`, `getControl`, `getTargets`, `getControls`, `getParameters`) are all fully delegated to `getBodyUnitary()`. This means `qc::InvOp` correctly exposes a control/target split (inherited from the wrapped unitary) under reference semantics. By contrast, `qco::InvOp` has no such split because all qubits are flat operands under linear/value typing. This semantic divergence is intentional and benign: canonicalization guarantees `InvOp` always wraps a plain zero-control unitary in practice, so `getBodyUnitary().getNumControls() == 0` always holds in well-formed IR.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:203-204
Timestamp: 2026-03-31T10:05:06.230Z
Learning: In `mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` (PR `#1580`), the fix for the `qtensorMap`-keyed-on-raw-alloc-result issue was implemented by adding `target.addDynamicallyLegalDialect<memref::MemRefDialect>()` with a predicate that marks any memref op illegal if it touches a `memref<...x!qc.qubit>` type (checking both operand types and result types). This replaces multiple individual `addDynamicallyLegalOp` calls. Any unsupported qubit-bearing memref op (e.g. `memref.cast`) now fails with a clear conversion error rather than triggering an assert inside a pattern.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:855-864
Timestamp: 2026-03-30T16:51:08.373Z
Learning: In `mlir/lib/Conversion/QCOToQC/QCOToQC.cpp` (PR `#1580`), `target.addIllegalDialect<qtensor::QTensorDialect>()` is intentionally broader than the current pattern coverage. Conversion patterns for `qtensor::FromElementsOp`, `qtensor::ExtractSliceOp`, and `qtensor::InsertSliceOp` are deliberately absent because these ops are not yet used in the pipeline. They are tracked for addition in a future follow-up PR. Do not flag missing patterns for these three ops as a bug until that follow-up is addressed.
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1542
File: mlir/lib/Dialect/QTensor/IR/Operations/ExtractSliceOp.cpp:28-39
Timestamp: 2026-03-13T12:59:28.174Z
Learning: In the munich-quantum-toolkit/core repository, standard MLIR operation lifecycle methods — `build()`, `verify()`, `fold()`, and `getCanonicalizationPatterns()` — defined in operation implementation files (e.g., under `mlir/lib/Dialect/*/IR/Operations/`) do NOT require Doxygen-style comment blocks. This is consistent with the existing QCO dialect operation definitions and is the established project convention. The general Doxygen requirement (learning 26aa0c0f) applies to non-boilerplate C++ code but is explicitly waived for these standard MLIR op definition methods.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1580
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1313-1335
Timestamp: 2026-03-30T16:53:34.674Z
Learning: In `mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` (PR `#1580`), the dynamic legality predicates for `memref::AllocOp`, `memref::LoadOp`, and `memref::DeallocOp` (and their corresponding conversion patterns `ConvertMemRefAllocOp`, `ConvertMemRefLoadOp`, `ConvertMemRefDeallocOp`) do NOT yet include boundary checks to detect or reject `memref<...x!qc.qubit>` values that cross `func`, `scf.if`, or `scf.while` boundaries. This is an intentional limitation: multiple-function and SCF support are not yet implemented in this pass. The boundary checks (restricting qubit-container memrefs to `scf.for` only, consistent with the constraint that only `scf.for` may carry qubit containers) are deferred until PR `#1396` is merged. Do not flag the absence of these boundary checks in the current PR.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1479
File: mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp:53-68
Timestamp: 2026-03-07T21:26:36.904Z
Learning: In the QCOToJeff conversion pass (mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp), the LoweringState struct intentionally uses a single-frame modifier representation (two booleans: inCtrlOp/inInvOp, plus single op fields ctrlOp/invOp). The pass explicitly assumes at most one level of modifier nesting in the form `qco.ctrl { qco.inv { ... } }` and raises an error (signals failure) if any other nesting pattern or deeper nesting is encountered. This is a known, enforced limitation for this first version of the pass.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1330
File: mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp:126-139
Timestamp: 2026-02-09T15:19:47.622Z
Learning: In the QC dialect (mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp), InvOp::verify intentionally restricts the body region to exactly two operations (one unitary + yield). Multi-op region inversion is by-design deferred to future work, potentially via a dedicated Unitary Operation for grouping gates.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1407
File: mlir/lib/Compiler/CompilerPipeline.cpp:140-144
Timestamp: 2026-04-16T00:51:46.153Z
Learning: In `mlir/lib/Compiler/CompilerPipeline.cpp` and `mlir/include/mlir/Compiler/CompilerPipeline.h` (PR `#1407`), the `MergeSingleQubitRotationGates` pass is intentionally **enabled by default** in the compiler pipeline. `QuantumCompilerConfig::disableMergeSingleQubitRotationGates` defaults to `false` (pass ON), and the CLI exposes `--disable-merge-single-qubit-rotation-gates` as an opt-out escape hatch. Do not flag the default-on polarity as incorrect or inconsistent with an "opt-in" rollout — the design was deliberately changed to default-on in the course of the PR's development.
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: 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.
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks a lot for getting this started, @J4MMlE! This is already looking pretty good to me! 🙂
I like how clean the additions to the builders and the conversions are! The tests also look really good to me already!
You can find some more detailed comments below. Most of them should be rather easy to resolve!
| ``` | ||
| }]; | ||
|
|
||
| let arguments = (ins F64Attr:$exponent); |
There was a problem hiding this comment.
Do we want to allow attributes here? I feel like it would be more consistent to allow only operands (see, e.g., the inputs to rotation gates). @burgholzer, do you have an opinion here? 🤔
| let arguments = (ins F64Attr:$exponent); | |
| let arguments = (ins F64:$exponent); |
There was a problem hiding this comment.
If we decide to go with this, the builders should accept std::variant<double, Value>.
There was a problem hiding this comment.
Hm. Does the current version even work with a non-compile-time-known power?
Pretty sure we need to accept a Value/F64 here to make this work.
The consistent solution would be to only accept that.
We could make an exception here since this is a modifier and not a regular gate operation, but then again, I do not really see why that would be justified.
| /** | ||
| * @brief Raise a unitary operation to a power | ||
| * | ||
| * @param qubits Input qubits (targets for the powered operation) | ||
| * @param exponent The power exponent (floating-point) | ||
| * @param body Function that builds the body containing the target operation | ||
| * @return Output qubits |
There was a problem hiding this comment.
I'd prefer to keep this docstring consistent with the one in QCProgramBuilder.h (apart from the actual differences between QC and QCO).
| ``` | ||
| }]; | ||
|
|
||
| let arguments = (ins F64Attr:$exponent, |
There was a problem hiding this comment.
Same comment here.
| let arguments = (ins F64Attr:$exponent, | |
| let arguments = (ins F64:$exponent, |
| /** | ||
| * @brief Absorb inv into pow by negating the exponent, i.e., | ||
| * `inv(pow(p){U}) => pow(-p){U}`. | ||
| * | ||
| * This is always valid for unitaries: (U^p)† = U^{-p}. | ||
| * Downstream patterns (e.g., NegPowToInvPow) can then rewrite | ||
| * pow(-p){U} => pow(p){inv(U)} when the exponent is an integer. | ||
| */ | ||
| struct MovePowOutside final : OpRewritePattern<InvOp> { |
There was a problem hiding this comment.
The pattern name seems to be a bit misleading. 🤔
| * @brief Absorb inv into pow by negating the exponent, i.e., | ||
| * `inv(pow(p){U}) => pow(-p){U}`. | ||
| * | ||
| * This is always valid for unitaries: (U^p)† = U^{-p}. |
There was a problem hiding this comment.
| * This is always valid for unitaries: (U^p)† = U^{-p}. | |
| * @details This is always valid for unitaries: (U^p)† = U^{-p}. |
| // pow(1/3){SX} inside ctrl: fold would emit gphase+rx (two ops, | ||
| // invalid in ctrl body) → pow survives and hits ConvertQCOPowOp | ||
| QCOToQCTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qco::ctrlPowSx), | ||
| MQT_NAMED_BUILDER(qc::ctrlPowSx)})); |
There was a problem hiding this comment.
Just to be more in line with the other test cases.
| // pow(1/3){SX} inside ctrl: fold would emit gphase+rx (two ops, | |
| // invalid in ctrl body) → pow survives and hits ConvertQCOPowOp | |
| QCOToQCTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qco::ctrlPowSx), | |
| MQT_NAMED_BUILDER(qc::ctrlPowSx)})); | |
| QCOToQCTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qco::ctrlPowSx), | |
| MQT_NAMED_BUILDER(qc::ctrlPowSx)})); |
| // pow(1/3){SX} inside ctrl: fold would emit gphase+rx (two ops, | ||
| // invalid in ctrl body) → pow survives and hits ConvertQCPowOp | ||
| QCToQCOTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qc::ctrlPowSx), | ||
| MQT_NAMED_BUILDER(qco::ctrlPowSx)})); |
There was a problem hiding this comment.
| // pow(1/3){SX} inside ctrl: fold would emit gphase+rx (two ops, | |
| // invalid in ctrl body) → pow survives and hits ConvertQCPowOp | |
| QCToQCOTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qc::ctrlPowSx), | |
| MQT_NAMED_BUILDER(qco::ctrlPowSx)})); | |
| QCToQCOTestCase{"CtrlPowSx", MQT_NAMED_BUILDER(qc::ctrlPowSx), | |
| MQT_NAMED_BUILDER(qco::ctrlPowSx)})); |
| INSTANTIATE_TEST_SUITE_P( | ||
| QCPowOpTest, QCTest, | ||
| testing::Values( | ||
| // pow(1) { g } => g (InlinePow1) |
There was a problem hiding this comment.
Same comment here: I don't know how necessary the additional comments are. We usually don't put them. I'll leave it up to you.
| /// Creates a circuit with pow(1.0) modifier (should inline to just the gate). | ||
| void pow1Inline(QCProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with pow(0.0) modifier (should erase to identity). | ||
| void pow0Erase(QCProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with nested pow modifiers (should merge exponents). | ||
| void nestedPow(QCProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with pow(6.0) as the merged reference for nestedPow. | ||
| void powSingleExponent(QCProgramBuilder& b); |
There was a problem hiding this comment.
For consistency, the test case names may include the gate being used. This would make it easier to understand what pow1Inline canonicalizes to.
| rewriter.replaceOpWithNewOp<PowOp>( | ||
| op, op.getQubitsIn(), -exp, | ||
| [&](ValueRange powArgs) -> llvm::SmallVector<Value> { | ||
| return InvOp::create( | ||
| rewriter, op.getLoc(), powArgs, | ||
| [&](ValueRange invArgs) -> llvm::SmallVector<Value> { | ||
| auto* invBody = rewriter.getInsertionBlock(); | ||
| rewriter.inlineBlockBefore(op.getBody(), invBody, | ||
| invBody->begin(), invArgs); | ||
| auto yieldedValues = | ||
| llvm::to_vector(invBody->back().getOperands()); | ||
| rewriter.eraseOp(&invBody->back()); | ||
| return yieldedValues; | ||
| }) | ||
| .getResults(); | ||
| }); | ||
|
|
||
| return success(); |
There was a problem hiding this comment.
It looks like this is not covered by any tests. 🤔
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just batching up some comments to back up Daniel's review.
| ``` | ||
| }]; | ||
|
|
||
| let arguments = (ins F64Attr:$exponent); |
There was a problem hiding this comment.
Hm. Does the current version even work with a non-compile-time-known power?
Pretty sure we need to accept a Value/F64 here to make this work.
The consistent solution would be to only accept that.
We could make an exception here since this is a modifier and not a regular gate operation, but then again, I do not really see why that would be justified.
| template <typename GateOp> | ||
| static LogicalResult replaceOneTargetOneParam(auto theta, PowOp op, | ||
| PatternRewriter& rewriter) { | ||
| rewriter.replaceOpWithNewOp<GateOp>(op, op.getTarget(0), theta); | ||
| return success(); | ||
| } | ||
|
|
||
| template <typename GateOp> | ||
| static LogicalResult replaceTwoTargetsOneParam(auto theta, PowOp op, | ||
| PatternRewriter& rewriter) { | ||
| rewriter.replaceOpWithNewOp<GateOp>(op, op.getTarget(0), op.getTarget(1), | ||
| theta); | ||
| return success(); | ||
| } | ||
|
|
||
| template <typename GateOp> | ||
| static LogicalResult replaceOneTargetTwoParams(auto theta, auto phi, PowOp op, | ||
| PatternRewriter& rewriter) { | ||
| rewriter.replaceOpWithNewOp<GateOp>(op, op.getTarget(0), theta, phi); | ||
| return success(); | ||
| } | ||
|
|
||
| template <typename GateOp> | ||
| static LogicalResult replaceTwoTargetsTwoParams(auto theta, auto beta, PowOp op, | ||
| PatternRewriter& rewriter) { | ||
| rewriter.replaceOpWithNewOp<GateOp>(op, op.getTarget(0), op.getTarget(1), | ||
| theta, beta); | ||
| return success(); | ||
| } |
There was a problem hiding this comment.
Not too helpful I suppose. Inlining all of these would probably reduce the number of lines and templates that need to be instantiated.
| if (llvm::isa<CtrlOp, InvOp, PowOp>(op->getParentOp())) { | ||
| if (op.getNumTargets() == 1) { | ||
| rewriter.replaceOpWithNewOp<IdOp>(op, op.getTarget(0)); | ||
| } else { | ||
| return failure(); | ||
| } | ||
| } else { | ||
| rewriter.eraseOp(op); | ||
| } | ||
| return success(); |
There was a problem hiding this comment.
Hm. Given how we can infer that the operation is to be deleted, we may not want to rely on yet another program traversal to catch that.
| rewriter.moveOpBefore(innerPow, op); | ||
| innerPow->setOperands(op.getInputQubits()); | ||
| innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); | ||
| rewriter.replaceOp(op, innerPow->getResults()); |
There was a problem hiding this comment.
@J4MMlE I believe @coderabbitai might have a point here.
denialhaag
force-pushed
the
pow-modifier
branch
from
71115a4 to
afb4008
Compare
denialhaag
force-pushed
the
pow-modifier
branch
from
afb4008 to
40a42f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
mlir/include/mlir/Dialect/QC/IR/QCOps.td (1)
1020-1023:⚠️ Potential issue | 🟡 MinorTighten negative-exponent semantics in the PowOp description.
Line 1022 presents
r < 0as a universal rewrite toinv @ pow(-r) @ g; that is only safe for integer exponents. Please clarify the integer-only rewrite condition in the description.📝 Suggested wording
- - r < 0: equivalent to inv @ pow(-r) @ g. + - r < 0: represented as a negative exponent. + Canonical rewrite to pow(-r){inv{g}} is only applied for integer r.Based on learnings,
NegPowToInvPowinmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppis mathematically correct only for integer exponents and is guarded accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td` around lines 1020 - 1023, The PowOp documentation currently states "r < 0: equivalent to inv @ pow(-r) @ g" without scoping; update the description in QCOps.td so the negative-exponent rewrite is only claimed for integer exponents (e.g., "r < 0 and r is an integer: equivalent to inv @ pow(-r) @ g"); mention that the transformation is implemented/guarded by the NegPowToInvPow modifier (see NegPowToInvPow in mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp) to make the integer-only condition explicit.mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp (1)
228-233:⚠️ Potential issue | 🟠 MajorWrap the
innerPowedits inmodifyOpInPlace.After
moveOpBefore, the directsetOperands/setExponentAttrcalls bypass rewriter notifications. Ifmergedbecomes0or1, the greedy driver may never requeueinnerPow, soErasePow0/InlinePow1can be missed.♻️ Proposed fix
rewriter.moveOpBefore(innerPow, op); - innerPow->setOperands(op.getInputQubits()); - innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); + rewriter.modifyOpInPlace(innerPow, [&] { + innerPow->setOperands(op.getInputQubits()); + innerPow.setExponentAttr(rewriter.getF64FloatAttr(merged)); + }); rewriter.replaceOp(op, innerPow->getResults());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp` around lines 228 - 233, After moving innerPow, don't call innerPow->setOperands or innerPow.setExponentAttr directly; instead wrap the mutation in rewriter.modifyOpInPlace(innerPow, [&](auto &opRef){ ... }) so the rewriter is notified and the op is requeued for passes like ErasePow0/InlinePow1; moveOpBefore(op, innerPow) remains, compute merged as before, then inside modifyOpInPlace update operands and exponent attribute (use rewriter.getF64FloatAttr(merged)), and finally call rewriter.replaceOp(op, innerPow->getResults()).
🤖 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/QCOToQC/QCOToQC.cpp`:
- Around line 721-748: Extract the duplicated block-argument remap/erase logic
into a shared helper (e.g., remapAndEraseBlockArguments) and call it from this
lowering and the existing ConvertQCOInvOp/ConvertQCOCtrlOp implementations: the
helper should take the cloned entry Block, the adaptor operand list
(adaptor.getQubitsIn() / ArrayRef<Value>), and the rewriter/op being modified
(so it can call modifyOpInPlace on qcOp), perform the replaceAllUsesWith loop
over entryBlock.getArgument(i) -> newArgs[i], then eraseArguments(0, numArgs) if
numArgs>0; replace the inline lambda in this file with a call to that helper to
avoid duplicated logic and divergence.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 231-233: The comment for the InvPowRx test is incorrect: replace
the invalid canonicalization comment "inv { pow(p) { g } } => pow(p) { inv { g }
}" with the safe identity "inv(pow(p){g}) => pow(-p){g}" and update the test
description for QCOTestCase "InvPowRx" (builders invPowRx / powRxNeg)
accordingly; also ensure the canonicalization implementation in InvOp's
MovePowOutside uses the algebraic rule (U^p)^{-1} = U^{-p} (i.e., negate the
exponent) rather than transforming into (U^{-1})^p in the InvOp/MovePowOutside
code in mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp and
mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp.
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 1020-1023: The PowOp documentation currently states "r < 0:
equivalent to inv @ pow(-r) @ g" without scoping; update the description in
QCOps.td so the negative-exponent rewrite is only claimed for integer exponents
(e.g., "r < 0 and r is an integer: equivalent to inv @ pow(-r) @ g"); mention
that the transformation is implemented/guarded by the NegPowToInvPow modifier
(see NegPowToInvPow in mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp) to make the
integer-only condition explicit.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 228-233: After moving innerPow, don't call innerPow->setOperands
or innerPow.setExponentAttr directly; instead wrap the mutation in
rewriter.modifyOpInPlace(innerPow, [&](auto &opRef){ ... }) so the rewriter is
notified and the op is requeued for passes like ErasePow0/InlinePow1;
moveOpBefore(op, innerPow) remains, compute merged as before, then inside
modifyOpInPlace update operands and exponent attribute (use
rewriter.getF64FloatAttr(merged)), and finally call rewriter.replaceOp(op,
innerPow->getResults()).
🪄 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: 6c95cafa-47a7-4631-8b43-2ec44acf4d56
📒 Files selected for processing (14)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QC/IR/Modifiers/PowOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cpp
| // Clone body region from QCO to QC | ||
| auto& dstRegion = qcOp.getRegion(); | ||
| rewriter.cloneRegionBefore(op.getRegion(), dstRegion, dstRegion.end()); | ||
|
|
||
| auto& entryBlock = dstRegion.front(); | ||
| const auto numArgs = entryBlock.getNumArguments(); | ||
| if (adaptor.getQubitsIn().size() != numArgs) { | ||
| return op.emitOpError() << "qco.pow: entry block args (" << numArgs | ||
| << ") must match number of target operands (" | ||
| << adaptor.getQubitsIn().size() << ")"; | ||
| } | ||
|
|
||
| // Remove all block arguments in the cloned region | ||
| rewriter.modifyOpInPlace(qcOp, [&] { | ||
| // 1. Replace uses (Must be done BEFORE erasing) | ||
| for (auto i = 0UL; i < numArgs; ++i) { | ||
| entryBlock.getArgument(i).replaceAllUsesWith(adaptor.getQubitsIn()[i]); | ||
| } | ||
|
|
||
| // 2. Erase all block arguments | ||
| if (numArgs > 0) { | ||
| entryBlock.eraseArguments(0, numArgs); | ||
| } | ||
| }); | ||
|
|
||
| // Replace the output qubits with the same QC references | ||
| rewriter.replaceOp(op, adaptor.getOperands()); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract shared helper for cloned-region arg remapping.
The block-argument remap/erase sequence here duplicates logic already present in ConvertQCOInvOp (and similar flow in ConvertQCOCtrlOp). Consider extracting a small helper to reduce divergence risk when modifier lowering evolves.
🤖 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 721 - 748, Extract the
duplicated block-argument remap/erase logic into a shared helper (e.g.,
remapAndEraseBlockArguments) and call it from this lowering and the existing
ConvertQCOInvOp/ConvertQCOCtrlOp implementations: the helper should take the
cloned entry Block, the adaptor operand list (adaptor.getQubitsIn() /
ArrayRef<Value>), and the rewriter/op being modified (so it can call
modifyOpInPlace on qcOp), perform the replaceAllUsesWith loop over
entryBlock.getArgument(i) -> newArgs[i], then eraseArguments(0, numArgs) if
numArgs>0; replace the inline lambda in this file with a call to that helper to
avoid duplicated logic and divergence.
| // inv { pow(p) { g } } => pow(p) { inv { g } } | ||
| QCOTestCase{"InvPowRx", MQT_NAMED_BUILDER(invPowRx), | ||
| MQT_NAMED_BUILDER(powRxNeg)}, |
There was a problem hiding this comment.
Fix the inv(pow(...)) canonicalization comment.
Line 231 currently states inv { pow(p) { g } } => pow(p) { inv { g } }, which is not generally valid for fractional exponents; the safe canonical identity is inv(pow(p){g}) => pow(-p){g}.
✏️ Suggested comment update
- // inv { pow(p) { g } } => pow(p) { inv { g } }
+ // inv { pow(p) { g } } => pow(-p) { g }Based on learnings, in mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp and mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp, MovePowOutside must use (U^p)^{-1} = U^{-p} and not (U^{-1})^p for non-integer exponents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 231 - 233, The
comment for the InvPowRx test is incorrect: replace the invalid canonicalization
comment "inv { pow(p) { g } } => pow(p) { inv { g } }" with the safe identity
"inv(pow(p){g}) => pow(-p){g}" and update the test description for QCOTestCase
"InvPowRx" (builders invPowRx / powRxNeg) accordingly; also ensure the
canonicalization implementation in InvOp's MovePowOutside uses the algebraic
rule (U^p)^{-1} = U^{-p} (i.e., negate the exponent) rather than transforming
into (U^{-1})^p in the InvOp/MovePowOutside code in
mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp and
mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp.
|
@J4MMlE Just as an FYI: I am slowly clearing out my backlog of review requests and might soon have some time to look at this again. At the moment, there still seem to be quite some open comments from the last round of reviews. I expect #1638 to land before this PR does, which might prompt some changes in the conversions between both dialects. I hope the conflicts will boil down to a minimum. |
|
@J4MMlE may I ask for an update here? It would be great to move this along 😌 |
|
Sorry for letting this become stale for so long. I'll be back at it next week! |
3 participants
Description
Adds a power modifier (
pow) to both MLIR-dialectsQCandQCO.The power modifier raises an operation
Uby an integer/floating numberkpow(k) { U } == U^kin other words the corresponding unitary matrix gets raised by factorkvia eigendecomposition:U = V D V† => U^p = V D^p V†.When
kis an integerU^kcould also be unrolled (Uis then appliedk-times to the input qubit - but this modifier does NOT implement any unrolling - this is left for (possibly) later stages in the pipeline).QC
QCO
Canonicalizations
The modifier contains the following canonicalizations in both dialects:
pow(a) { pow(b) { U } } → pow(a*b) { U }pow(1) { U } → Upow(0) { U } → remove (becomes identity, then eliminated)pow(-r) { U } → pow(r) { inv { U } }pow { ctrl { U } } → ctrl { pow { U } }inv:inv { pow { U } } -> pow { inf { U } }Gate specific canonicalizations
pow(r) { G(θ, ...) } → G(r·θ, ...)G in {GPhase, RX, RY, RZ, P, RXX, RYY, RZX, RZZ, R(θ,φ), XX+YY(θ,β), XX-YY(θ,β)}P-Gates:Z=P(π),S=P(π/2),Sdg=P(-π/2),T=P(π/4),Tdg=P(-π/4):pow(r) { G }→ named gate for normalizedr*base_angle, elseP(r*base_angle)All gates are implicitly converted and scaled as a P gate i.e.
G^r = P(r*base_angle). Then we check ifr*base_angleis again a base angle of a named gate: matchr*base_anglewith:0 → erase,±π → Z,π/2 → S,-π/2 → Sdg,π/4 → T,-π/4 → TdgX/Y:pow(r) { X/Y } → gphase(-r·π/2); RX/RY(r·π)Xonly:r=½ → SX,r=-½ → SXdgSX/SXdg— symmetric pair:|r| = 2 → Xpow(r) { SX } → gphase(-r·π/4); RX( r·π/2)pow(r) { SXdg } → gphase(+r·π/4); RX(-r·π/2)pow(n) { H / ECR / SWAP } → erasefor even exponentelse
→ H / ECR / SWAPfor odd exponentiSWAPpow(r) { iSWAP } → XX+YY(-r·π, 0)pow(r) { Id / Barrier } → Id / BarrierNotes
jeffdialectInvOpandPowOpis duplicated (extracting operations, reordering, etc. is basically the same) - maybe its a good idea to unify by creating utility functions, which all modifiers can use - this might be only worth doing, if more modifiers would be implemented (which is unlikely).pow(r) { inv { U }}topow(-r) { U }we leave it as is and instead convert the other way around (see Negative Power Canonicalization)Related Issues
Fixes #1129
Fixes #1131
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.