✨ 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, canonicalization/folding and unitary-matrix support, math helpers, and broad unit-test/program-builder coverage for pow semantics. ChangesPowOp modifier
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 qc.pow (exponent, region)
QCPow->>Converter: request conversion / lowering
Converter->>QCOBuilder: create qco.pow (exponent), clone region
QCOBuilder->>QCOBuilder: build region block args, yield outputs
Converter->>QCPow: return converted op mapping
QCPow->>Canon: enqueue for canonicalization
Canon->>Canon: apply folding/rewrites (scale params, inline/erase, merge)
alt folded
Canon->>User: replace with simplified ops
else unchanged
Canon->>User: leave PowOp
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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✨ Simplify code
|
![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. |
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.
|
@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! |
J4MMlE
force-pushed
the
pow-modifier
branch
2 times, most recently
from
4cd390a to
06f1ab5
Compare
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks for addressing all of my comments! 🙂
I briefly had a look at some of your changes and noticed a small detail, but it shouldn't be too big an issue to address this.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
mlir/include/mlir/Dialect/QC/IR/QCOps.td (1)
1016-1022:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify negative fractional-power semantics in the op description.
Line 1022 still documents
r < 0asinv @ pow(-r) @ gunconditionally, but that rewrite is only sound for integer exponents. For fractional negative powers, the canonical form must stay aspow(-r){U}because principal-branch semantics differ on unitaries with eigenvalue-1(e.g.H,X,Y,Z,ECR,SWAP).Based on learnings,
NegPowToInvPowis only mathematically correct for integer exponents and negative fractional exponents must not be documented as equivalent topow(-r){inv{U}}.🤖 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/IR/QCOps.td` around lines 1016 - 1022, Update the QCOps.td description for the modifier operation to clarify negative-power semantics: state that the rewrite "r < 0: inv @ pow(-r) @ g" is only valid for integer exponents and must not be applied to fractional negative exponents; for negative fractional r the canonical form remains pow(-r){U} because principal-branch semantics can differ for unitaries with eigenvalue -1 (e.g. H, X, Y, Z, ECR, SWAP); also reference the transformation rule name NegPowToInvPow to indicate it is restricted to integer exponents.mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp (1)
166-203:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftClone the full body when rebuilding nested modifiers.
NegPowToInvPow,MergeNestedPow, andMoveCtrlOutsideonly clonegetBodyUnitary(). For cases likepow(-2) { rx(0.123) }, thearith.constantfor0.123lives in the old body, so the cloned gate in the new modifier still references a value from the region being erased. That produces invalid IR and reintroduces the dangling-value failure you already handled inFoldPowIntoGate.Please inline/move the non-
YieldOpbody ops together with the unitary, or clone/remap the unitary’s operand-def chain into the new body before replacingop.🤖 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/PowOp.cpp` around lines 166 - 203, NegPowToInvPow, MergeNestedPow and MoveCtrlOutside currently only clone getBodyUnitary() into the newly created modifier body which leaves upstream ops (e.g. constants) referenced from the old region and causes dangling-value IR; fix each pattern (the matchAndRewrite implementations that call rewriter.replaceOpWithNewOp and PowOp::create / InvOp::create) to also clone or move the non-Yield ops that the unitary depends on into the new region (or clone/remap the unitary’s operand-def chain) before replacing the original op so all SSA operands in the new body point to values inside the new region (similar to the work done in FoldPowIntoGate but applied here to preserve constants and other defs).
🤖 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/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 489-535: PowOp currently assumes the exponent is a constant in
getExponentValue(), but verify() doesn't enforce that; update PowOp::verify() to
reject dynamic exponents by checking getExponent() is an arith.constant (use
matchPattern(getExponent(), m_Constant(&attr)) or equivalent) and emit an op
error (e.g., "exponent must be a constant") when it is not, so
getExponentValue() is only called on verified ops; reference
PowOp::getExponentValue, PowOp::verify, getExponent(), and
matchPattern/m_Constant.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp`:
- Around line 624-683: PowOp::verify must reject non-constant exponents to avoid
hard aborts in downstream canonicalizations; update PowOp::verify to check that
the exponent operand (getExponent()) is a constant floating-point value (the
same form expected by getExponentValue()/getUnitaryMatrix()) and emitOpError if
it is not, so IR with non-constant exponents cannot be constructed, or
alternatively adjust getExponentValue() callers to gracefully bail out—prefer
the constants-only fix by adding a const-check in PowOp::verify referencing
getExponent() and getExponentValue() to locate the relevant logic.
In `@mlir/unittests/Dialect/QC/IR/test_qc_ir.cpp`:
- Around line 149-157: Add the missing regression tests that cover fractional
exponents and the control-modifier context: add a QCTestCase that exercises
inv(pow(0.5){H}) and expects pow(-0.5){H} (use the appropriate builders, e.g.,
invPowHFrac and powHFracNeg) and add a QCTestCase that instantiates ctrlPowSx
with its reference (e.g., ctrlPowSx and ctrlPowSxRef) so the ctrl { pow(1/3){sx}
} path stays blocked; ensure these new cases reference the same test harness
symbols as the others and rely on MovePowOutside emitting pow(-p){U} (not
wrapping in inv).
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 255-275: Add the missing regression case for the multi-op
expansion path by adding the ctrlPowSx test into the INSTANTIATE_TEST_SUITE_P
list: create a QCOTestCase entry using MQT_NAMED_BUILDER(ctrlPowSx) and the
appropriate reference builder (e.g., MQT_NAMED_BUILDER(ctrlPowSxRef) or the
existing reference name from qco_programs.cpp) so the INSTANTIATE_TEST_SUITE_P
(QCOPowOpTest, QCOTest, ...) covers the ctrlPowSx scenario that triggers
sx->gphase+rx expansion and validates the PowOp-in-modifier bug.
- Around line 267-268: The test case using QCOTestCase{"NegPowH",
MQT_NAMED_BUILDER(negPowH), MQT_NAMED_BUILDER(negPowH)} incorrectly
self-compares; change it to assert structure instead of using negPowH as the
reference so the rewrite NegPowToInvPow cannot hide an invalid transformation.
Update the "NegPowH" test to run the pipeline on negPowH and then verify the
cleaned IR still contains a pow node with a negative fractional exponent applied
to H (e.g., pow(-0.5){ h } or equivalent AST node), rather than comparing
against the same builder; locate the QCOTestCase entry for "NegPowH" and replace
the reference builder with an assertion helper that inspects the resulting IR
for a pow node with exponent -0.5 and base H (or fails if an inv-based form
appears).
- Around line 263-264: The test compares powRxx to itself so a broken
angle-scaling rewrite would still pass; update the QCOTestCase entry for
"PowRxx" to use a distinct reference builder that emits the already-scaled rxx
(i.e., rxx(2θ)) instead of MQT_NAMED_BUILDER(powRxx) twice. Replace the second
MQT_NAMED_BUILDER(powRxx) with a dedicated builder function name (e.g.,
powRxxScaled or powRxxRef) that constructs the expected canonical form, or
supply an inline/reference builder that produces rxx with the scaled angle;
ensure the unique symbol you add is used in the test entry so the case compares
the rewritten result to the correct scaled reference.
In `@mlir/unittests/programs/qco_programs.h`:
- Around line 1184-1212: Add a regression case that constructs inv(pow(0.5){H})
and asserts it canonicalizes to the same form produced by negPowH rather than to
pow(0.5){inv{H}}; specifically, in the test suite create a new helper/test that
uses QCOProgramBuilder to build inv(pow(0.5){H}), run the
canonicalization/passes that trigger InvPowToNegPow, and compare the result
against negPowH (reuse negPowH as the expected reference); ensure the test
references the inv(pow(...){...}) source form and the negPowH reference so the
new rewrite is exercised and validated.
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 1016-1022: Update the QCOps.td description for the modifier
operation to clarify negative-power semantics: state that the rewrite "r < 0:
inv @ pow(-r) @ g" is only valid for integer exponents and must not be applied
to fractional negative exponents; for negative fractional r the canonical form
remains pow(-r){U} because principal-branch semantics can differ for unitaries
with eigenvalue -1 (e.g. H, X, Y, Z, ECR, SWAP); also reference the
transformation rule name NegPowToInvPow to indicate it is restricted to integer
exponents.
In `@mlir/lib/Dialect/QC/IR/Modifiers/PowOp.cpp`:
- Around line 166-203: NegPowToInvPow, MergeNestedPow and MoveCtrlOutside
currently only clone getBodyUnitary() into the newly created modifier body which
leaves upstream ops (e.g. constants) referenced from the old region and causes
dangling-value IR; fix each pattern (the matchAndRewrite implementations that
call rewriter.replaceOpWithNewOp and PowOp::create / InvOp::create) to also
clone or move the non-Yield ops that the unitary depends on into the new region
(or clone/remap the unitary’s operand-def chain) before replacing the original
op so all SSA operands in the new body point to values inside the new region
(similar to the work done in FoldPowIntoGate but applied here to preserve
constants and other defs).
🪄 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: 2ab9a6c3-b681-421d-bb07-b876cdca1610
📒 Files selected for processing (23)
CHANGELOG.mdmlir/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
| double PowOp::getExponentValue() { | ||
| FloatAttr attr; | ||
| if (!matchPattern(getExponent(), m_Constant(&attr))) { | ||
| reportFatalUsageError("PowOp exponent must be a constant"); | ||
| } | ||
| return attr.getValueAsDouble(); | ||
| } | ||
|
|
||
| UnitaryOpInterface PowOp::getBodyUnitary() { | ||
| return cast<UnitaryOpInterface>(*(++getBody()->rbegin())); | ||
| } | ||
|
|
||
| void PowOp::build(OpBuilder& odsBuilder, OperationState& odsState, | ||
| const std::variant<double, Value>& exponent, | ||
| const function_ref<void()>& bodyBuilder) { | ||
| auto expValue = variantToValue(odsBuilder, odsState.location, exponent); | ||
| odsState.addOperands(expValue); | ||
| auto* region = odsState.addRegion(); | ||
| auto& block = region->emplaceBlock(); | ||
|
|
||
| const OpBuilder::InsertionGuard guard(odsBuilder); | ||
| odsBuilder.setInsertionPointToStart(&block); | ||
| bodyBuilder(); | ||
| YieldOp::create(odsBuilder, odsState.location); | ||
| } | ||
|
|
||
| LogicalResult PowOp::verify() { | ||
| auto& block = *getBody(); | ||
| if (block.getOperations().size() < 2) { | ||
| return emitOpError("body region must have at least two operations"); | ||
| } | ||
| if (!isa<YieldOp>(block.back())) { | ||
| return emitOpError( | ||
| "last operation in body region must be a yield operation"); | ||
| } | ||
| auto iter = ++block.rbegin(); | ||
| if (!isa<UnitaryOpInterface>(*iter)) { | ||
| return emitOpError( | ||
| "second to last operation in body region must be a unitary operation"); | ||
| } | ||
| for (auto it = ++iter; it != block.rend(); ++it) { | ||
| if (isa<UnitaryOpInterface>(*it)) { | ||
| return emitOpError("body region may only contain a single unitary op"); | ||
| } | ||
| } | ||
| return success(); | ||
| } |
There was a problem hiding this comment.
Reject dynamic exponents or make the patterns skip them.
Line 492 turns a non-constant exponent into a fatal error, but PowOp::build and PowOp::verify still accept any Value exponent. All canonicalizations above call getExponentValue() unconditionally, so a valid pow(%r) can crash the rewrite pipeline instead of just remaining unreduced.
Either tighten verify() to require an arith.constant exponent, or have the patterns use valueToDouble(getExponent()) and return failure() when the exponent is dynamic.
🤖 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/PowOp.cpp` around lines 489 - 535, PowOp
currently assumes the exponent is a constant in getExponentValue(), but verify()
doesn't enforce that; update PowOp::verify() to reject dynamic exponents by
checking getExponent() is an arith.constant (use matchPattern(getExponent(),
m_Constant(&attr)) or equivalent) and emit an op error (e.g., "exponent must be
a constant") when it is not, so getExponentValue() is only called on verified
ops; reference PowOp::getExponentValue, PowOp::verify, getExponent(), and
matchPattern/m_Constant.
| LogicalResult PowOp::verify() { | ||
| auto& block = *getBody(); | ||
| if (block.getOperations().size() < 2) { | ||
| return emitOpError("body region must have at least two operations"); | ||
| } | ||
| const auto numTargets = getNumTargets(); | ||
| if (block.getArguments().size() != numTargets) { | ||
| return emitOpError( | ||
| "number of block arguments must match the number of targets"); | ||
| } | ||
| const auto qubitType = QubitType::get(getContext()); | ||
| for (size_t i = 0; i < numTargets; ++i) { | ||
| if (block.getArgument(i).getType() != qubitType) { | ||
| return emitOpError("block argument type at index ") | ||
| << i << " does not match target type"; | ||
| } | ||
| } | ||
| if (!isa<YieldOp>(block.back())) { | ||
| return emitOpError( | ||
| "last operation in body region must be a yield operation"); | ||
| } | ||
| if (const auto numYieldOperands = block.back().getNumOperands(); | ||
| numYieldOperands != numTargets) { | ||
| return emitOpError("yield operation must yield ") | ||
| << numTargets << " values, but found " << numYieldOperands; | ||
| } | ||
| auto iter = ++block.rbegin(); | ||
| if (!isa<UnitaryOpInterface>(*iter)) { | ||
| return emitOpError( | ||
| "second to last operation in body region must be a unitary operation"); | ||
| } | ||
| for (auto it = ++iter; it != block.rend(); ++it) { | ||
| if (isa<UnitaryOpInterface>(*it)) { | ||
| return emitOpError("body region may only contain a single unitary op"); | ||
| } | ||
| } | ||
|
|
||
| auto bodyUnitary = getBodyUnitary(); | ||
| if (bodyUnitary.getNumQubits() != numTargets) { | ||
| return emitOpError("body unitary must operate on exactly ") | ||
| << numTargets << " target qubits, but found " | ||
| << bodyUnitary.getNumQubits(); | ||
| } | ||
| const auto numQubits = bodyUnitary.getNumQubits(); | ||
| for (size_t i = 0; i < numQubits; i++) { | ||
| if (bodyUnitary.getInputQubit(i) != block.getArgument(i)) { | ||
| return emitOpError("body unitary must use target alias block argument ") | ||
| << i << " (and not the original target operand)"; | ||
| } | ||
| } | ||
|
|
||
| // Also require yield to forward the unitary's outputs in-order. | ||
| for (size_t i = 0; i < numTargets; ++i) { | ||
| if (block.back().getOperand(i) != bodyUnitary.getOutputQubit(i)) { | ||
| return emitOpError("yield operand ") | ||
| << i << " must be the body unitary output qubit " << i; | ||
| } | ||
| } | ||
|
|
||
| return success(); |
There was a problem hiding this comment.
Reject non-constant exponents before these rewrites run.
QCOProgramBuilder::pow(...) and the new PowOp builders accept Value exponents, so pow(%arg){...} is currently constructible and passes this verifier. But the canonicalizations in this file and getUnitaryMatrix() all funnel through getExponentValue(), which hard-aborts on non-constants. That turns otherwise well-formed IR into a compiler crash during canonicalization/matrix queries instead of a normal verification or rewrite failure. Either make verify() require a constant floating-point exponent for now, or change getExponentValue()/its callers to bail out gracefully.
Minimal safe fix if constants-only is the intended contract
LogicalResult PowOp::verify() {
+ FloatAttr exponentAttr;
+ if (!matchPattern(getExponent(), m_Constant(&exponentAttr))) {
+ return emitOpError("exponent must be a constant floating-point value");
+ }
+
auto& block = *getBody();
if (block.getOperations().size() < 2) {
return emitOpError("body region must have at least two operations");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cpp` around lines 624 - 683,
PowOp::verify must reject non-constant exponents to avoid hard aborts in
downstream canonicalizations; update PowOp::verify to check that the exponent
operand (getExponent()) is a constant floating-point value (the same form
expected by getExponentValue()/getUnitaryMatrix()) and emitOpError if it is not,
so IR with non-constant exponents cannot be constructed, or alternatively adjust
getExponentValue() callers to gracefully bail out—prefer the constants-only fix
by adding a const-check in PowOp::verify referencing getExponent() and
getExponentValue() to locate the relevant logic.
| QCTestCase{"NegPowH", MQT_NAMED_BUILDER(negPowH), | ||
| MQT_NAMED_BUILDER(negPowH)}, | ||
| QCTestCase{"InvPowRx", MQT_NAMED_BUILDER(invPowRx), | ||
| MQT_NAMED_BUILDER(powRxNeg)}, | ||
| QCTestCase{"PowCtrlRx", MQT_NAMED_BUILDER(powCtrlRx), | ||
| MQT_NAMED_BUILDER(ctrlPowRx)}, | ||
| QCTestCase{"NegPowInvIswap", | ||
| MQT_NAMED_BUILDER(negPowInvIswap), | ||
| MQT_NAMED_BUILDER(negPowInvIswapRef)})); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add the missing modifier-context regression cases.
InvPowRx only covers an integer exponent, and this suite never instantiates ctrlPowSx. That leaves the two fragile paths from this PR unguarded: inv(pow(0.5){H}) should canonicalize to pow(-0.5){H}, and ctrl { pow(1/3){sx} } should stay blocked from decomposing into a multi-op modifier body.
Based on learnings, MovePowOutside must emit pow(-p){U} for fractional exponents rather than wrapping the body in inv.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/Dialect/QC/IR/test_qc_ir.cpp` around lines 149 - 157, Add the
missing regression tests that cover fractional exponents and the
control-modifier context: add a QCTestCase that exercises inv(pow(0.5){H}) and
expects pow(-0.5){H} (use the appropriate builders, e.g., invPowHFrac and
powHFracNeg) and add a QCTestCase that instantiates ctrlPowSx with its reference
(e.g., ctrlPowSx and ctrlPowSxRef) so the ctrl { pow(1/3){sx} } path stays
blocked; ensure these new cases reference the same test harness symbols as the
others and rely on MovePowOutside emitting pow(-p){U} (not wrapping in inv).
| INSTANTIATE_TEST_SUITE_P( | ||
| QCOPowOpTest, QCOTest, | ||
| testing::Values(QCOTestCase{"Pow1Inline", MQT_NAMED_BUILDER(pow1Inline), | ||
| MQT_NAMED_BUILDER(rx)}, | ||
| QCOTestCase{"Pow0Erase", MQT_NAMED_BUILDER(pow0Erase), | ||
| MQT_NAMED_BUILDER(emptyQCO)}, | ||
| QCOTestCase{"NestedPow", MQT_NAMED_BUILDER(nestedPow), | ||
| MQT_NAMED_BUILDER(powSingleExponent)}, | ||
| QCOTestCase{"PowRxx", MQT_NAMED_BUILDER(powRxx), | ||
| MQT_NAMED_BUILDER(powRxx)}, | ||
| QCOTestCase{"NegPowRx", MQT_NAMED_BUILDER(negPowRx), | ||
| MQT_NAMED_BUILDER(powRxNeg)}, | ||
| QCOTestCase{"NegPowH", MQT_NAMED_BUILDER(negPowH), | ||
| MQT_NAMED_BUILDER(negPowH)}, | ||
| QCOTestCase{"InvPowRx", MQT_NAMED_BUILDER(invPowRx), | ||
| MQT_NAMED_BUILDER(powRxNeg)}, | ||
| QCOTestCase{"PowCtrlRx", MQT_NAMED_BUILDER(powCtrlRx), | ||
| MQT_NAMED_BUILDER(ctrlPowRx)}, | ||
| QCOTestCase{"NegPowInvIswap", | ||
| MQT_NAMED_BUILDER(negPowInvIswap), | ||
| MQT_NAMED_BUILDER(negPowInvIswapRef)})); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
The suite is still missing the ctrlPowSx regression case.
The hard PowOp-in-modifier bug for this PR only shows up when pow would expand into multiple ops, e.g. sx -> gphase + rx. The current PowCtrlRx / CtrlPowRx entries stay single-op, and ctrlPowSx from qco_programs.cpp is not instantiated here, so that path is still untested.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 255 - 275, Add
the missing regression case for the multi-op expansion path by adding the
ctrlPowSx test into the INSTANTIATE_TEST_SUITE_P list: create a QCOTestCase
entry using MQT_NAMED_BUILDER(ctrlPowSx) and the appropriate reference builder
(e.g., MQT_NAMED_BUILDER(ctrlPowSxRef) or the existing reference name from
qco_programs.cpp) so the INSTANTIATE_TEST_SUITE_P (QCOPowOpTest, QCOTest, ...)
covers the ctrlPowSx scenario that triggers sx->gphase+rx expansion and
validates the PowOp-in-modifier bug.
| QCOTestCase{"PowRxx", MQT_NAMED_BUILDER(powRxx), | ||
| MQT_NAMED_BUILDER(powRxx)}, |
There was a problem hiding this comment.
PowRxx currently can't fail for a broken angle-scaling rewrite.
This compares powRxx against itself, so the case still passes if pow(2){rxx(θ)} forgets to canonicalize to rxx(2θ)}. Please use a distinct reference builder that emits the scaled rxx directly.
Suggested wiring
- QCOTestCase{"PowRxx", MQT_NAMED_BUILDER(powRxx),
- MQT_NAMED_BUILDER(powRxx)},
+ QCOTestCase{"PowRxx", MQT_NAMED_BUILDER(powRxx),
+ MQT_NAMED_BUILDER(powRxxRef)},+void powRxxRef(QCOProgramBuilder& b) {
+ auto q = b.allocQubitRegister(2);
+ b.rxx(2.0 * 0.123, q[0], q[1]);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 263 - 264, The
test compares powRxx to itself so a broken angle-scaling rewrite would still
pass; update the QCOTestCase entry for "PowRxx" to use a distinct reference
builder that emits the already-scaled rxx (i.e., rxx(2θ)) instead of
MQT_NAMED_BUILDER(powRxx) twice. Replace the second MQT_NAMED_BUILDER(powRxx)
with a dedicated builder function name (e.g., powRxxScaled or powRxxRef) that
constructs the expected canonical form, or supply an inline/reference builder
that produces rxx with the scaled angle; ensure the unique symbol you add is
used in the test entry so the case compares the rewritten result to the correct
scaled reference.
| QCOTestCase{"NegPowH", MQT_NAMED_BUILDER(negPowH), | ||
| MQT_NAMED_BUILDER(negPowH)}, |
There was a problem hiding this comment.
NegPowH needs a structural assertion, not a self-comparison.
This is the exact fractional-negative case that must not go through NegPowToInvPow. Using negPowH as both program and reference runs the same cleanup twice, so an invalid rewrite would still pass. Please make this a dedicated test that asserts the cleaned IR still contains pow(-0.5){ h } rather than an inv-based form.
Based on learnings, NegPowToInvPow is only mathematically sound for integer exponents; negative fractional powers of Hermitian gates like H must remain as pow(-r){U}.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 267 - 268, The
test case using QCOTestCase{"NegPowH", MQT_NAMED_BUILDER(negPowH),
MQT_NAMED_BUILDER(negPowH)} incorrectly self-compares; change it to assert
structure instead of using negPowH as the reference so the rewrite
NegPowToInvPow cannot hide an invalid transformation. Update the "NegPowH" test
to run the pipeline on negPowH and then verify the cleaned IR still contains a
pow node with a negative fractional exponent applied to H (e.g., pow(-0.5){ h }
or equivalent AST node), rather than comparing against the same builder; locate
the QCOTestCase entry for "NegPowH" and replace the reference builder with an
assertion helper that inspects the resulting IR for a pow node with exponent
-0.5 and base H (or fails if an inv-based form appears).
| /// Creates a circuit with pow(-0.5) wrapping H (negative non-integer exponent). | ||
| /// Expected to remain unchanged: fractional exponent on a unitary with | ||
| /// eigenvalue -1 cannot safely apply NegPowToInvPow. | ||
| void negPowH(QCOProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with inv wrapping pow (should reorder to pow wrapping | ||
| /// inv). | ||
| void invPowRx(QCOProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with pow wrapping ctrl wrapping RX (should move ctrl | ||
| /// outside). | ||
| void powCtrlRx(QCOProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with ctrl wrapping pow wrapping RX (reference for | ||
| /// powCtrlRx). | ||
| void ctrlPowRx(QCOProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with pow(-2) wrapping inv wrapping iSWAP. | ||
| /// Exercises NegPowToInvPow: inv{iswap} survives InvOp canonicalization, | ||
| /// FoldPowIntoGate fails (inner is InvOp), so NegPowToInvPow fires. | ||
| void negPowInvIswap(QCOProgramBuilder& b); | ||
|
|
||
| /// Reference for negPowInvIswap: xx_plus_yy(-2π, 0) (the fully folded form). | ||
| void negPowInvIswapRef(QCOProgramBuilder& b); | ||
|
|
||
| /// Creates a circuit with ctrl wrapping pow(1/3) wrapping SX. The fold | ||
| /// pow(p){SX} → gphase+rx is suppressed inside ctrl (would emit two ops), | ||
| /// so the pow survives canonicalization and reaches ConvertQCOPowOp. | ||
| void ctrlPowSx(QCOProgramBuilder& b); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a regression case for inv(pow(0.5){H}).
This header already adds negPowH, but the new InvPowToNegPow rewrite is only really pinned if you also exercise the source form that triggers it: inv(pow(0.5){H}) should canonicalize to that same negative-fractional pow form, not to pow(0.5){inv{H}}. Reusing negPowH as the reference would make this a very small but high-value coverage addition.
Based on learnings: inv(pow(p){U}) must be rewritten by negating the exponent, and negative fractional powers on Hermitian/self-inverse gates must remain as pow(-r){U} because the inverse-inside-power form is not mathematically sound.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mlir/unittests/programs/qco_programs.h` around lines 1184 - 1212, Add a
regression case that constructs inv(pow(0.5){H}) and asserts it canonicalizes to
the same form produced by negPowH rather than to pow(0.5){inv{H}}; specifically,
in the test suite create a new helper/test that uses QCOProgramBuilder to build
inv(pow(0.5){H}), run the canonicalization/passes that trigger InvPowToNegPow,
and compare the result against negPowH (reuse negPowH as the expected
reference); ensure the test references the inv(pow(...){...}) source form and
the negPowH reference so the new rewrite is exercised and validated.
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
I have added migration instructions to the upgrade guide (if needed).If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.