✨ Implement two-qubit gate decomposition pass #1206
Conversation
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
d7f49cc to
4bc6f15
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
c4f6399 to
ffb96f3
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
2e3a48f to
f5ae2b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
3 times, most recently
from
bbcec1e to
49d512f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new gate decomposition pass is added to the MLIR MQTOpt dialect. The implementation includes pass declaration, Weyl-based two-qubit decomposition pattern logic, utility helpers for MLIR/quantum operations, CMake integration with Eigen dependency, and comprehensive MLIR tests. Changes
Sequence DiagramsequenceDiagram
participant Pass as GateDecomposition Pass
participant Pattern as GateDecompositionPattern
participant Decomposer as TwoQubitBasisDecomposer
participant Rewriter as PatternRewriter
Pass->>Pass: runOnOperation()
Pass->>Pass: Create RewritePatternSet
Pass->>Pattern: populateGateDecompositionPatterns()
Pattern->>Pass: Register pattern
Pass->>Pass: Configure GreedyRewriteConfig
loop For each Operation (top-down)
Pass->>Pattern: matchAndRewrite(UnitaryInterface op)
alt Pattern Matches
Pattern->>Decomposer: create() + twoQubitDecompose()
Decomposer->>Decomposer: Weyl decomposition
Decomposer->>Decomposer: Generate gate sequence
Decomposer-->>Pattern: Return TwoQubitSeries
Pattern->>Pattern: applySeries() - replace + validate
Pattern->>Rewriter: Emit new operations
Rewriter-->>Pass: Update IR
else No Match
Pattern-->>Pass: Skip operation
end
end
alt Success
Pass->>Pass: Pass succeeded
else Failure
Pass->>Pass: Signal pass failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(1 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h(1 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.td(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/CMakeLists.txt(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/GateDecomposition.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Helpers.h(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.tdmlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/GateDecomposition.cppmlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.hmlir/lib/Dialect/MQTOpt/Transforms/Helpers.hmlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/Helpers.h
📚 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/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/Helpers.hmlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
CMakeLists.txt
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
[warning] 1689-1689: mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp:1689:30 [misc-include-cleaner]
no header providing "std::numeric_limits" is directly included
[warning] 929-929: mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp:929:14 [misc-include-cleaner]
no header providing "Eigen::Vector" is directly included
⏰ 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). (5)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐉 Test / 🍎 macos-15-intel with LLVM@21
🔇 Additional comments (3)
mlir/lib/Dialect/MQTOpt/Transforms/CMakeLists.txt (1)
10-10: Confirm thatEigen3::Eigenis the correct target from your Eigen FetchContent setupLinking against
Eigen3::Eigenhere is fine as long as the FetchContent configuration really defines that target on all supported toolchains; otherwise this will fail to configure or link in some environments. Please double‑check the Eigen CMake export to ensure the target name and namespace match what you use here (or add an alias/guard if necessary).mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h (1)
38-38: New pattern population hook looks consistent with existing transformsThe
populateGateDecompositionPatterns(RewritePatternSet&)declaration fits cleanly alongside the other populate* helpers and matches the usage inGateDecomposition.cpp. Just make sure the implementation inGateDecompositionPattern.cppis actually compiled intoMLIRMQTOptTransformsso the call site links correctly.mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp (1)
460-540: Creation ofGPhaseOpignores potential control qubits.When applying a series, the pattern adds a global phase gate via:
if (sequence.hasGlobalPhase()) { createOneParameterGate<GPhaseOp>(rewriter, location, sequence.globalPhase, {}); }This always creates a GPhase with empty
inQubits, even if the original series might have had controlled global phases. Given the dialect definition allowsGPhaseOpto be controlled by qubits, this may drop control wiring when decomposing controlled global-phase structures. (mqt.readthedocs.io)Please double‑check whether controlled GPhaseOps can appear in practice. If they can, you likely want to thread the relevant control qubits into the newly created
GPhaseOp(and its types) instead of using an emptyValueRange.⛔ Skipped due to learnings
Learnt from: DRovara Repo: munich-quantum-toolkit/core PR: 1108 File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288 Timestamp: 2025-10-09T13:20:11.483Z Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.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.Learnt from: burgholzer Repo: munich-quantum-toolkit/core PR: 1283 File: src/qir/runtime/QIR.cpp:196-201 Timestamp: 2025-11-01T15:57:31.153Z Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
flowerthrower
left a comment
flowerthrower
left a comment
There was a problem hiding this comment.
Thank you @taminob for all your work. The math and logic appear to be correctly implemented. I loosely checked the expected K1, K2, ... decomposition values with a small debug script on my Mac and was able to reproduce (roughtly) the same values as the Qiskit decomposition.
The three remaining points are:
- Set up a proper test to automate exactly this verification process.
- Improve comments — just because the Rust implementation is poorly documented does not mean we should do the same.
Especially since parts of your code are already nicely commented, it makes sense to extend this good style to the copied code. - Revisit MLIR logic after dialect revamp
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
2 times, most recently
from
7fc67db to
18cf6b0
Compare
|
I reworked the tests now (added unittests and made the MLIR tests more robust/less precise). A couple of questions:
|
I just skimmed through both the lit tests and the unit tests and I think the overall direction is fine. I am pretty sure that I will have some more detailed comments when I check the PR in more detail.
Hm. multiple thoughts on this.
These two might actually be more or less the same, the first might just be the second with some reasonable and relaxed defaults on the native gate-set.
I am not quite sure that I follow this. That link says:
We are using more modern compilers than this (by far). The current CI runs report
This seems to indicate to me that you might need to dig a little deeper there. An example of how that Attr is constructed would be This might not be final yet; mostly because I still want to explore some options that might allow us to make the gates/gate matrices singletons and avoid repeated constructions of matrices for gates where this is definitely not necessary.
I am not too familiar with the details of |
The documentation of I think it might be mainly about the stability of the interfaces - so it could happen that different Eigen versions will break API/ABI. However, since they are user-provided, I guess it also heavily depends on the actual function to be used (see also this stackoverflow question.
I'll look into it, although it's a bit hard on a different architecture. Could well be that it is caused by differences in alignment on ARM. |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
4 times, most recently
from
a9d1fc9 to
7efae47
Compare
Unfortunately, I couldn't really find the source of the error - also because I didn't manage to get a stacktrace in the CI and so can't really pinpoint the issue. I tried using C++23 I think the cause might be one of https://libeigen.gitlab.io/eigen/docs-nightly/group__TopicPassingByValue.html or https://libeigen.gitlab.io/eigen/docs-nightly/group__TopicStlContainers.html (which should be fixed since C++11, but maybe LLVM+MSVC behaves buggy here - who knows) which are quite serious and need to be considered when using Eigen (at least I wasn't aware of it). I'd suggest to simply set |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
2 times, most recently
from
888deb2 to
7a80d30
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
f45852b to
c818f56
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
b6c4301 to
ce79ed2
Compare
…rror at end and wrong result
| auto traces = this->traces(targetDecomposition); | ||
| auto getDefaultNbasis = [&]() { | ||
| auto minValue = std::numeric_limits<fp>::min(); | ||
| auto minIndex = -1; | ||
| for (int i = 0; std::cmp_less(i, traces.size()); ++i) { | ||
| // lower fidelity means it becomes easier to choose a lower number of | ||
| // basis gates | ||
| auto value = helpers::traceToFidelity(traces[i]) * | ||
| std::pow(actualBasisFidelity, i); | ||
| if (value > minValue) { | ||
| minIndex = i; | ||
| minValue = value; | ||
| } | ||
| } | ||
| return minIndex; |
There was a problem hiding this comment.
note to myself: check again, should probably be named maxValue/maxIndex and see if there is some llvm helper function for this
Cpp-Linter Report
|
taminob
added
enhancement
Description
TODO
Related to #1122
Checklist: