✨ 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. |
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Builder as Flux/Quartz<br/>Builder
participant InvOp
participant Canonicalizer
User->>Builder: inv(targets, body_fn)
Builder->>InvOp: create with targets &<br/>body region
InvOp->>InvOp: verify body structure<br/>& qubit uniqueness
rect rgb(240, 248, 255)
Note over Canonicalizer: Canonicalization phase
Canonicalizer->>InvOp: detect inv(inv(x))?
alt Nested inverse found
Canonicalizer->>InvOp: extract inner body
Canonicalizer->>InvOp: replace outer inv<br/>with inner unitary
Canonicalizer->>InvOp: erase inner inv
Canonicalizer-->>User: simplified to x
else Single or non-nested
Canonicalizer-->>User: unchanged
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
Cpp-Linter Report
|
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. |
Description
Adds a
quartz.invandflux.invOp, similar to the existingCtrlOp.Example:
Quartz:
Flux:
Missing / Unclear things:
inv(s) -> sdgQuartz -> QIRlowering forquartz.inv. I think we assume that the canonicalizations already ran and the onlyinvs left are those for which we don't know the inverse. In that case, we can only error.This PR depends on #1264.
Checklist: