✨ Implement single- and two-qubit gate decomposition pass#1206
✨ Implement single- and 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.
|
|
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:
📝 WalkthroughWalkthroughAdds a gate-decomposition pass and a decomposition subsystem (Weyl, basis, Euler), numeric/matrix helpers, TableGen/CMake/pass wiring for a new QcoPasses library, compiler-pipeline hookup, and comprehensive unit tests for decomposition components. Changes
Sequence Diagram(s)sequenceDiagram
participant PM as PassManager
participant GD as GateDecompositionPass
participant WD as TwoQubitWeylDecomposition
participant BD as TwoQubitBasisDecomposer
participant ED as EulerDecomposition
participant GS as GateSequence
PM->>GD: run on Module
activate GD
loop each 1‑2 qubit candidate
GD->>GD: assemble 4x4 unitary
alt two-qubit
GD->>WD: create(unitary, fidelity)
WD-->>GD: (a,b,c, k1/k2, phase)
GD->>BD: create(basisGate, fidelity)
BD-->>GD: TwoQubitGateSequence
BD->>ED: synthesize 1q pieces
ED-->>BD: OneQubitGateSequence
else single-qubit
GD->>ED: generateCircuit(basis, unitary)
ED-->>GD: OneQubitGateSequence
end
GD->>GS: assemble & apply sequence (incl. global phase)
end
GD-->>PM: rewritten Module
deactivate GD
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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: 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
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mlir/lib/Passes/CMakeLists.txt`:
- Around line 13-24: The add_mlir_library invocation for QcoPasses has an extra
PRIVATE token before DEPENDS which incorrectly suggests DEPENDS and
QcoPassesIncGen are link items; remove the redundant PRIVATE so DEPENDS and
QcoPassesIncGen are specified as top-level arguments. Locate the
add_mlir_library block that defines QcoPasses and adjust it by deleting the
second PRIVATE line immediately preceding DEPENDS, leaving DEPENDS
QcoPassesIncGen as standalone arguments.
In `@mlir/lib/Passes/Patterns/GateDecompositionPattern.cpp`:
- Around line 622-644: The loop over sequence.gates in applySeries only handles
X (as a CX), RX, RY, RZ and falls back to llvm::reportFatalInternalError; extend
the dispatch to cover other expected basis gates (e.g., H, S, T, Z, CZ, SWAP,
controlled variants) by adding new else-if branches that: for single-qubit gates
call the appropriate helper (e.g., addSingleQubitGate or reuse
addSingleQubitRotationGate where applicable) and then call
updateInQubits(gate.qubitId, newGate); for multi-qubit/controlled gates use
createControlledGate<...>(...) like the existing createControlledGate<XOp>
pattern and preserve the unitaryMatrix update via
decomposition::fixTwoQubitMatrixQubitOrder when relevant; finally remove or
limit llvm::reportFatalInternalError to unreachable debug-only assertion and
instead log/return an explicit unsupported-gate error so unknown future basis
gates won't crash at runtime (refer to sequence.gates, gate.type,
createControlledGate<XOp>, addSingleQubitRotationGate, updateInQubits,
unitaryMatrix, decomposition::fixTwoQubitMatrixQubitOrder).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/lib/Passes/CMakeLists.txt`:
- Around line 45-46: Replace the unconditional PUBLIC definition of
EIGEN_MAX_STATIC_ALIGN_BYTES=16 in the target_compile_definitions call for
QcoPasses with a platform-conditional definition that only sets the macro on
impacted targets (e.g., Windows ARM/others that caused the CI failures),
preserving PUBLIC scope when applied; do the same conditional guard for the
identical definition in the MLIRQCODialect target so both places (the
target_compile_definitions entries for QcoPasses and MLIRQCODialect) only inject
EIGEN_MAX_STATIC_ALIGN_BYTES=16 on the specific platforms rather than globally,
ensuring AVX/AVX-512 hosts keep full alignment for Eigen types exposed in public
headers like mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h.
In `@mlir/unittests/Passes/Decomposition/test_weyl_decomposition.cpp`:
- Around line 64-73: TestApproximation currently passes the magic literal 1.0 -
1e-12 into TwoQubitWeylDecomposition::create but then compares restoredMatrix
with originalMatrix using the default isApprox tolerance (same as TestExact),
making the distinction unclear; replace the literal with a named constant (e.g.,
kNearPerfectFidelity = 1.0 - 1e-12) and add a one-line comment above
TEST_P(WeylDecompositionTest, TestApproximation) explaining that this value
represents a near-perfect fidelity to exercise the "approximate" code path, or
alternatively add a small explicit assertion that demonstrates the test is
exercising the approximation branch (if TwoQubitWeylDecomposition exposes such a
flag), referencing TwoQubitWeylDecomposition::create, TestApproximation,
TestExact, and isApprox to locate the change.
- Around line 54-62: Change local bindings that currently use const auto& for
temporaries returned by GetParam()() to take them by value instead; specifically
in the TEST_P(WeylDecompositionTest, TestExact) test replace const auto&
originalMatrix = GetParam()(); (and the similar binding later on around the
restore/compare use) with const auto originalMatrix = GetParam()(); so
temporaries returned by GetParam()() are owned by the local variable (affecting
the variables used with TwoQubitWeylDecomposition::create and restore).
|
@burgholzer this PR should be ready for your review. I'm sorry this PR is so huge, but it contains now (as discussed before) both the logic for single- and two-qubit decomposition. To avoid bloating this PR even more (I think this also makes the coderabbit reviews worse), I did not add more extensive tests - I'd maybe suggest doing this in a follow-up in #1499 where coderabbit generated some more edge case test cases. I'm not sure about how to test it in the context of the pipeline since it will be hard to test passes in isolation once we start adding lots of optimizations to the pipeline. I'll also mention you in all of the remaining open coderabbit comments since the history of this PR is quite long by now. Once this PR is merged and we agree on the project structure for optimizations, I'd start working on the swap-reconstruction-and-elision pass. |
taminob
changed the title
|
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. |
|
FYI, the WireIterator is back (#1510) and now again available for the QCO dialect. |
…t-gate-decomposition-pass
…t-gate-decomposition-pass
|
I updated the branch and got stuck on some new segfaults due to alignment issues in Eigen again (no idea why the merge caused this). I don't think this needs to block the review, the issue is already merged in
I had a look at this and played around with it a little bit. |
You should not have to pass this option at all, anywhere.
I suspect that a very similar thing is going on here. That being said, the setting has been successfully removed on main and all tests and builds work like a charm.
👍 okay. |
Not sure if there is something wrong with my system, but I experience inconsistent crashes due to alignment issues - I'm using my thesis evaluation script which runs the
Maybe it worked before because we didn't actually use matrix operations on the returned matrices? |
4 participants
Description
This PR introduces a decomposition pass that is used as an optimization to shorten a one- or two-qubit gate series if possible.
Related to #1122
Checklist: