✨ Add inv Modifier for MLIR Redesign#1330
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Builder as Builder (QC/QCO)
participant InvOp as InvOp (qc/qco.inv)
participant Canon as Canonicalizer
participant Conv as Converter (QC↔QCO)
participant Pipeline as Compiler Pipeline
Builder->>InvOp: create region body (block of unitary ops)
InvOp->>InvOp: verify region (unitary-only, yields, no duplicates)
InvOp->>Canon: register canonicalization patterns
Pipeline->>Canon: run canonicalization
alt canonicalizable
Canon->>InvOp: apply per-op adjoints / cancel nested invs / move ctrl
else preserved
Canon-->>Pipeline: keep inv for later lowering
end
Pipeline->>Conv: lower/convert inv between QC and QCO
Conv->>InvOp: clone/translate region, wire block args/targets
Conv-->>Pipeline: lowered IR for downstream stages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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. 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Flux/IR/FluxOps.td(1 hunks)mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Quartz/IR/QuartzOps.td(1 hunks)mlir/include/mlir/Dialect/Utils/MatrixUtils.h(1 hunks)mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp(2 hunks)mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/FluxOps.cpp(4 hunks)mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/QuartzOps.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/include/mlir/Dialect/Quartz/IR/QuartzOps.tdmlir/include/mlir/Dialect/Flux/IR/FluxOps.tdmlir/lib/Dialect/Flux/IR/FluxOps.cppmlir/lib/Dialect/Quartz/IR/QuartzOps.cpp
🧬 Code graph analysis (6)
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (2)
mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (2)
inv(406-410)inv(407-407)mlir/lib/Dialect/Quartz/IR/QuartzOps.cpp (2)
invOp(379-392)invOp(379-380)
mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (2)
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (3)
inv(513-525)inv(513-515)ValueRange(479-489)mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (2)
inv(406-410)inv(407-407)
mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (1)
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (2)
inv(513-525)inv(513-515)
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (3)
QuartzProgramBuilder(32-34)inv(406-410)inv(407-407)mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (2)
inv(513-525)inv(513-515)
mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp (1)
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (17)
op(203-221)op(204-205)op(243-259)op(244-245)op(281-300)op(282-283)op(330-354)op(331-332)op(381-400)op(382-383)op(418-422)op(419-420)op(440-444)op(441-442)op(462-466)op(463-464)context(929-981)
mlir/lib/Dialect/Quartz/IR/QuartzOps.cpp (2)
mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (4)
getNumTargets(71-71)getNumControls(72-72)getNumPosControls(73-73)getNumNegControls(74-74)mlir/include/mlir/Dialect/Utils/MatrixUtils.h (1)
getMatrixAdj(181-211)
🪛 ast-grep (0.40.0)
mlir/lib/Dialect/Flux/IR/FluxOps.cpp
[warning] 356-356: The getTargetsIn function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsIn()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 358-358: The getTargetsOut function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsOut()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 378-378: The getTargetsOut function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsOut()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 379-379: The getTargetsIn function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsIn()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 387-387: The getTargetsIn function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsIn()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 388-388: The getTargetsOut function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsOut()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
🔇 Additional comments (4)
mlir/include/mlir/Dialect/Utils/MatrixUtils.h (1)
181-211: Implementation verified as correct and actively used; acknowledge known test gaps in PR objectives.The conjugate transpose implementation is mathematically correct and follows established patterns in the codebase. Verification confirms getMatrixAdj is actively used by InvOp::tryGetStaticMatrix() in both mlir/lib/Dialect/Quartz/IR/QuartzOps.cpp:347 and mlir/lib/Dialect/Flux/IR/FluxOps.cpp:404.
No existing tests were found for this function; however, this aligns with the PR objectives which explicitly acknowledge missing tests as a known limitation. Ensure test coverage is added as part of the PR's test implementation phase.
mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp (1)
650-686: ConvertFluxInvOp semantics look consistent with existing Ctrl conversionThe new
ConvertFluxInvOpcorrectly mirrors theCtrlOppattern: it creates aquartz::InvOp, clones the Flux body region into the Quartz op, and rewires the Fluxinvresults to the already-converted operands to model in-place semantics on Quartz. Registration in the pattern set is also consistent with the surrounding conversions. No changes needed.Also applies to: 761-762
mlir/lib/Dialect/Quartz/IR/QuartzOps.cpp (1)
282-348: Quartz InvOp implementation aligns with modifier semanticsThe added
InvOpbuilders, accessors, and verifier cleanly mirror the Ctrl-style modifier pattern: they encapsulate a single unitary op plusYieldOp, delegate all qubit/control/parameter queries to the body unitary, and derive the static matrix viagetMatrixAdjof the body unitary’s static matrix. This is consistent with the UnitaryOpInterface contract and Quartz’s in-place semantics.mlir/lib/Dialect/Flux/IR/FluxOps.cpp (1)
304-405: Flux InvOp implementation matches Ctrl-style modifier patternsThe Flux
InvOpbuilders, accessors, and verifier are structured analogously toCtrlOp: they construct a single-unitary body plusflux.yield, delegate all qubit/control/parameter queries to the inner unitary, and derive adjoint static matrices viagetMatrixAdjon the body’s static matrix. The verifier’s checks on body shape, yield arity, and uniqueness of input/output qubits align with the UnitaryOpInterface invariants. This looks solid.Also applies to: 407-442
Ectras
force-pushed
the
ectras/inv_modifier
branch
from
b5b7535 to
cbef1a9
Compare
Ectras
force-pushed
the
ectras/inv_modifier
branch
2 times, most recently
from
1da9a01 to
99fade4
Compare
Ectras
force-pushed
the
ectras/inv_modifier
branch
2 times, most recently
from
8a8c79e to
abb9346
Compare
Before, the passes tried to delete the innerInvOp but this failed as they still used the innerInnerUnitary. Now, the innerInnerUnitary is cloned and the operation can be deleted (happens within the replaceOp(...)).
Ectras
force-pushed
the
ectras/inv_modifier
branch
from
abb9346 to
1b72be6
Compare
|
Hey @Ectras 👋🏼 Canonicalizations would be great because I agree with your assessment that we can't really handle the inverse modifier in the QIR conversion because QIR has no such concept. Thus, these need to be resolved before that. This is something that I already observed for the CtrlOp modifier, but there will probably be quite a bit of duplication because the canonicalizations probably make sense in both dialects; modifiers only ever contain a single operation so it is equally easy to apply the canoncicalization in the QC dialect as in the QCO dialect. Just some initial thoughts though. The more we can unify, the better. The power modifier will introduce some similar code as well supposedly (with similar consequences). |
|
Thanks for the input @burgholzer! I think it makes sense to start simple and iterate as needed; e.g., first just do the canonicalization based on gate names (this is how the inverse operation is currently resolved in QuantumComputing) and later maybe introduce Hermitian or Rotation traits to allow for easier extensibility. Same for code duplication: I agree that there will be quite some duplication a) between the passes on the two dialects and b) between the different modifiers. I'd first get things to work and then refine where sensible. Implementing canonicalizations mainly in the QC dialect for now sounds reasonable. |
Fully agree. Just one correction: the canonicalizations can actually be done on the individual operation level not just with gate names. Each gate has its own class now and we do define some canonicalizations already for most of them. It should be pretty simple to adapt the convenience functions that @denialhaag already created for inverse cancellation and rotation merging. |
@burgholzer Thanks, and a happy new year to you, too 🎆 |
|
@burgholzer I think the PR is ready for review now |
|
Great to see this being ready for review 🎉 It's on my pile of PRs to go through. More likely than not, I am going to push some changes directly to the branch as I work through it. I'll try to get to the PR asap. |
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
@Ectras Thanks for working on this.
This was already looking really good before the commits I just pushed.
I fixed up a couple of things here and there (hope the individual commits are clear enough).
Let's see what the rabbit has to say, but otherwise this should be good to go for.
Testing itself is a little bit on the low end, but I wanted to expand the test suite a little bit anyway. So I will just include some more tests in a follow-up PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 1145-1211: The InvOp accessor methods for controls
(getNumControls, getInputControl, getOutputControl) should be declared static to
match BarrierOp and the zero-control pattern: update the extraClassDeclaration
in the InvOp TD to make getNumControls(), getInputControl(size_t), and
getOutputControl(size_t) static (keeping their return types and parameter
lists), and ensure any callers/use-sites expect static access (or adjust call
sites accordingly); this keeps the API consistent with BarrierOp while retaining
the same behavior if these methods simply return 0/assert.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp`:
- Around line 223-238: The CancelNestedInv::matchAndRewrite currently clones the
innerInnerUnitary but leaves its block arguments bound to the inner InvOp's
block args (out of scope); before calling rewriter.clone, create a
BlockAndValueMapping that maps innerInnerUnitary's block arguments to the outer
op's input targets (use op.getTargets() or the outerUnitary's operands/arguments
as the source values), apply that mapping when cloning (e.g.,
rewriter.clone(*innerInnerUnitary.getOperation(), mapping)), and then call
rewriter.replaceOp(op, clonedOp->getResults()); this ensures the cloned
operation's block args are remapped to the outer InvOp's inputs.
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mlir/lib/Dialect/QC/IR/Modifiers/InvOp.cpp`:
- Around line 126-139: The verifier InvOp::verify currently enforces exactly two
operations in the body (one unitary + yield) which blocks multi-op region
inversion; change the check to allow one-or-more unitary ops followed by a
single YieldOp: validate that the body has at least 2 operations, that the last
operation is a YieldOp, and that every operation before the last implements
UnitaryOpInterface (use block.getOperations(), block.back(),
block.front()/iterators to inspect). Update the error messages accordingly, and
then implement/update the lowering/canonicalization logic that handles
region-level inversion to reverse the order of these multiple unitary ops and
replace each with its adjoint (reverse+adjoint lowering/canonicalization for the
Inv region) so multi-op blocks are correctly inverted.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp`:
- Around line 75-216: The matchAndRewrite in InvOp's TypeSwitch is missing cases
for controlled-rotation ops (e.g., CRXOp/MCRXOp, CRYOp/MCRYOp, CRZOp/MCRZOp,
CRXXOp/MCRXXOp, etc.), so add .Case handlers for each controlled rotation that
create a negated angle via arith::NegFOp::create(rewriter, op.getLoc(),
<op>.getTheta()) and call rewriter.replaceOpWithNewOp<ThatControlledOp>(op,
/*preserve control operands and target operands in the same order*/
controlOperands..., targetOperands..., negTheta) (for multi-controlled variants
preserve the list of controls/targets similarly); update patterns for any ops
exposing getTheta()/getPhi()/getLambda() to negate the correct parameter(s) and
return success(). Ensure you reference matchAndRewrite, InvOp, TypeSwitch, and
use rewriter.replaceOpWithNewOp and arith::NegFOp to implement each case.
2 participants
Description
Adds a
qc.invandqco.invOp, similar to the existingCtrlOp.Example:
QC:
QCO:
Things to note:
inv { iSWAP }will currently error on the translation to QIR. A reasonable tactic could be to get the inverse unitary matrix and decompose it into native gates. I think this is outside the scope of this PR, though.Fixes #1130
Checklist: