✨ 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.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces a comprehensive quantum gate decomposition pass infrastructure for the MLIR-based quantum compiler. It adds decomposition algorithms for two-qubit gates (Weyl and basis decompositions), Euler angle-based single-qubit gate synthesis, supporting utilities for matrix operations, and integrates them into the compiler pipeline via a new GateDecompositionPass. Changes
Sequence Diagram(s)sequenceDiagram
participant PM as PassManager
participant GD as GateDecompositionPass
participant WD as WeylDecomposition
participant BD as BasisDecomposer
participant ED as EulerDecomposition
participant GS as GateSequence
PM->>GD: Process Module
activate GD
loop For each 2-qubit Gate in Module
GD->>WD: create(unitaryMatrix, fidelity)
activate WD
WD->>WD: Compute canonical form<br/>(magic basis, diagonalize)
WD->>WD: Select specialization<br/>(IdEquiv, SWAP, etc.)
WD-->>GD: TwoQubitWeylDecomposition
deactivate WD
GD->>BD: create(basisGate, fidelity)
activate BD
BD->>BD: Precompute basis<br/>decomposition matrices
BD-->>GD: TwoQubitBasisDecomposer
deactivate BD
GD->>BD: twoQubitDecompose(weylDecomp)
activate BD
BD->>BD: Select num basis gates<br/>(0, 1, 2, or 3)
BD->>ED: Synthesize 1-qubit gates<br/>via EulerBasis
activate ED
ED->>ED: Compute angles<br/>from unitary
ED-->>BD: OneQubitGateSequence
deactivate ED
BD->>BD: Construct gate<br/>combinations
BD-->>GD: TwoQubitGateSequence
deactivate BD
GD->>GS: Assemble final<br/>gate sequence
GD->>GS: Apply global phase
GS-->>GD: GateSequence
end
GD-->>PM: Rewritten Module
deactivate GD
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The diff introduces substantial quantum algorithm implementations across multiple files with dense mathematical logic (matrix diagonalization, Weyl decomposition, basis gate selection). While many files follow consistent patterns, the heterogeneous nature of decomposition algorithms (BasisDecomposer, WeylDecomposition, EulerDecomposition, Helpers) and their interdependencies require separate reasoning for each component. The integration into the compiler pipeline adds additional structural complexity. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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
Cpp-Linter Report
|
taminob
added
enhancement
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
264304a to
09bfcce
Compare
…t-gate-decomposition-pass
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
e7345db to
84a367a
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Passes/Decomposition/BasisDecomposer.h`:
- Around line 246-259: The lambda chooseDecomposition can return 2/3-basis
decompositions even when the basis isn't super-controlled; update it to guard on
bestNbasis for values 2 and 3 by checking the super-controlled capability
(compute or use the existing flag named like isSuperControlled or derive from
targetDecomposition) and if the basis is not super-controlled either clamp
bestNbasis to a supported value (e.g., 1) or throw std::logic_error when
approximate == false; implement the check before calling
decomp2Supercontrolled/decomp3Supercontrolled so that
decomp2Supercontrolled/decomp3Supercontrolled are only invoked when the basis
truly supports super-controlled decompositions, otherwise fallback to
decomp1(targetDecomposition) or raise an error.
In `@mlir/include/mlir/Passes/Decomposition/EulerBasis.h`:
- Around line 67-74: Add a brief comment above the switch/case handling the
EulerBasis enum (the cases for EulerBasis::PSX and EulerBasis::U1X that fall
through to the throw std::invalid_argument) explaining why PSX and U1X are
unsupported (e.g., incompatible parameterization, not needed for current
translation to gate types, or planned future work) and optionally add a TODO
with a reference to an issue/PR number if implementation is intended later;
place the comment next to the cases so future readers immediately see the
rationale for the thrown exception.
- Around line 15-16: Add the missing header <stdexcept> to EulerBasis.h so the
throw of std::invalid_argument used in the function that throws at the current
throw site is properly declared; update the includes at the top of
mlir/include/mlir/Passes/Decomposition/EulerBasis.h (alongside <cstdint> and
<llvm/ADT/SmallVector.h>) to include <stdexcept> to fix the compilation error
referencing std::invalid_argument.
In `@mlir/include/mlir/Passes/Decomposition/EulerDecomposition.h`:
- Around line 166-172: Document the intent of setting angleZeroEpsilon = -1.0 in
decomposeKAK: add a brief inline comment near the assignment explaining that
when simplify is false we set angleZeroEpsilon to -1.0 so subsequent comparisons
like std::abs(x) > angleZeroEpsilon always succeed (effectively disabling
zero-angle clipping), and reference DEFAULT_ATOL and the simplify flag to
clarify behavior for future maintainers.
- Around line 19-32: The header currently includes many MLIR-specific headers
that are unused and is missing <stdexcept> needed for std::invalid_argument;
remove unused includes (specifically drop <mlir/Dialect/Arith/IR/Arith.h>,
<mlir/IR/MLIRContext.h>, <mlir/IR/Operation.h>, <mlir/IR/PatternMatch.h>,
<mlir/IR/Value.h>, <mlir/IR/ValueRange.h>, <mlir/Support/LLVM.h>, and
<mlir/Support/LogicalResult.h>) and add `#include` <stdexcept> so code using
std::invalid_argument compiles cleanly while keeping necessary headers like
<Eigen/Core>, <array>, <cassert>, <cmath>, <complex>, and
<llvm/ADT/STLExtras.h>.
In `@mlir/include/mlir/Passes/Decomposition/GateSequence.h`:
- Around line 76-86: The assert in getUnitaryMatrix uses
helpers::isUnitaryMatrix without a tolerance and can fail due to floating-point
accumulation; update getUnitaryMatrix to validate unitarity with an explicit
numeric tolerance (e.g., call a helpers::isUnitaryMatrix overload that accepts
an epsilon or compute ||U*U† - I|| and compare to eps) and on failure either log
a warning including the computed deviation and the globalPhase or throw a clear
runtime_error depending on build expectations; specifically modify the assertion
around helpers::isUnitaryMatrix(unitaryMatrix) in getUnitaryMatrix to use a
tolerance-aware check and handle the non-unitary case gracefully.
In `@mlir/include/mlir/Passes/Decomposition/Helpers.h`:
- Around line 137-145: The getParameters function currently silently drops
non-constant parameters (via helpers::mlirValueToFp) which can cause downstream
indexing bugs; change its signature to return
std::optional<llvm::SmallVector<fp, 3>> and inside getParameters (for
UnitaryOpInterface op) attempt to fold every parameter using
helpers::mlirValueToFp(op.getParameter(i)), and if any parameter fails to fold
return std::nullopt immediately, otherwise return the filled SmallVector; update
call sites of getParameters to handle the optional result.
- Around line 17-29: Add the missing <functional> include to
mlir/include/mlir/Passes/Decomposition/Helpers.h because
performMlirFloatBinaryOp and performMlirFloatUnaryOp use std::invoke; update the
header by adding `#include` <functional> near the other includes so builds don't
rely on transitive includes and std::invoke is properly declared.
- Around line 185-187: The isUnitaryMatrix function currently calls
matrix.transpose().conjugate() * matrix).isIdentity() with Eigen's default
strict precision; change it to pass the project's tolerance constant
(SANITY_CHECK_PRECISION = 1e-12) to isIdentity so numerical rounding doesn’t
falsely fail. Update the implementation of isUnitaryMatrix (the function
template isUnitaryMatrix) to call .isIdentity(SANITY_CHECK_PRECISION) and ensure
SANITY_CHECK_PRECISION is referenced/imported where needed.
In `@mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h`:
- Around line 44-61: Add the missing [[nodiscard]] attribute to the inline
matrix factory functions rxMatrix, ryMatrix, and rzMatrix so they match the
other matrix functions; update each function signature to prepend [[nodiscard]]
(i.e., [[nodiscard]] inline matrix2x2 rxMatrix(...), [[nodiscard]] inline
matrix2x2 ryMatrix(...), [[nodiscard]] inline matrix2x2 rzMatrix(...)) to ensure
callers don't accidentally ignore the returned matrix.
- Around line 96-102: Turn these header-scope const matrix definitions into
inline variables to avoid ODR violations: change IDENTITY_GATE, SWAP_GATE,
H_GATE, IPZ, IPY, and IPX declarations to use the inline specifier (e.g., inline
const matrix2x2 IDENTITY_GATE = ... or inline constexpr if the matrix types are
constexpr-capable) so each translation unit can include the header safely;
update the six declarations (IDENTITY_GATE, SWAP_GATE, H_GATE, IPZ, IPY, IPX)
accordingly.
- Around line 131-166: The function getSingleQubitMatrix lacks the [[nodiscard]]
attribute; update its declaration by adding [[nodiscard]] (e.g., [[nodiscard]]
inline matrix2x2 getSingleQubitMatrix(const Gate& gate)) so callers are warned
when the returned matrix is ignored, keeping consistency with the rest of the
file; ensure you modify this exact function signature and no other logic.
- Around line 190-201: The three TODO comments questioning qubit order for the
symmetric two-qubit gates should be removed because RXX, RYY, and RZZ are
order-independent; update the block handling gate.type == qc::RXX / qc::RYY /
qc::RZZ by deleting the "// TODO: check qubit order?" comments adjacent to the
rxxMatrix(gate.parameter[0]), ryyMatrix(gate.parameter[0]), and
rzzMatrix(gate.parameter[0]) returns so the code reads cleanly and documents
that the current returns are correct.
In `@mlir/include/mlir/Passes/Decomposition/WeylDecomposition.h`:
- Around line 612-636: The code in the Specialization::SWAPEquiv branch
overwrites c to qc::PI_4 before testing its sign, so the negative-specialization
path can never run; fix it by preserving the original c sign (e.g., store double
originalC = c; before setting a=b=c = qc::PI_4) and then use originalC > 0.
rather than c > 0. when deciding whether to apply the flipped/global-phase
adjustments to k1l, k1r, globalPhase and k2l/k2r; refer to
newSpecialization/Specialization::SWAPEquiv, variables a,b,c and the gate/phase
updates to k1l,k1r,k2l,k2r and globalPhase (or alternately detect the negative
variant from bestSpecialization() and branch accordingly).
- Around line 39-175: The diagonal matrix exponentiation currently uses
Eigen::MatrixFunctions (temp = temp.exp()) which relies on unsupported headers;
instead compute the complex exponentials elementwise from dReal and IM and
populate temp's diagonal directly in TwoQubitWeylDecomposition::create (use
std::exp or std::polar on each scalar complex IM * dReal(i) to set temp(i,i)),
then remove the unsupported `#include` <unsupported/Eigen/MatrixFunctions>; ensure
temp remains a matrix4x4 with only the diagonal set to those exponentials and
off-diagonals zero.
In `@mlir/lib/Compiler/CompilerPipeline.cpp`:
- Around line 169-170: The cleanup passes are being run twice:
addCleanupPasses(pm) is invoked before recording afterOptimization (making
afterOptimizationCanon redundant) and then again in Stage 6; remove the first
addCleanupPasses call so cleanup is only applied in Stage 6, or alternatively
stop recording afterOptimizationCanon and only record afterOptimization after
Stage 6. Update the code around addOptimizationPasses(pm) / addCleanupPasses(pm)
and the recording of afterOptimization/afterOptimizationCanon so that
addCleanupPasses is invoked exactly once (refer to addOptimizationPasses,
addCleanupPasses, afterOptimization, and afterOptimizationCanon to locate and
adjust the logic).
| auto chooseDecomposition = [&]() { | ||
| if (bestNbasis == 0) { | ||
| return decomp0(targetDecomposition); | ||
| } | ||
| if (bestNbasis == 1) { | ||
| return decomp1(targetDecomposition); | ||
| } | ||
| if (bestNbasis == 2) { | ||
| return decomp2Supercontrolled(targetDecomposition); | ||
| } | ||
| if (bestNbasis == 3) { | ||
| return decomp3Supercontrolled(targetDecomposition); | ||
| } | ||
| throw std::logic_error{"Invalid basis to use"}; |
There was a problem hiding this comment.
Guard against 2/3-basis decompositions when the basis isn’t super‑controlled.
decomp2Supercontrolled/decomp3Supercontrolled only warn but still return gates, which can silently yield invalid decompositions (especially when approximate == false). Prefer failing fast or clamping bestNbasis.
🛠️ Suggested guard
// number of basis gates that need to be used in the decomposition
auto bestNbasis = numBasisGateUses.value_or(getDefaultNbasis());
+ if (!isSuperControlled && bestNbasis >= 2 && !approximate) {
+ return std::nullopt;
+ }🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/BasisDecomposer.h` around lines 246 -
259, The lambda chooseDecomposition can return 2/3-basis decompositions even
when the basis isn't super-controlled; update it to guard on bestNbasis for
values 2 and 3 by checking the super-controlled capability (compute or use the
existing flag named like isSuperControlled or derive from targetDecomposition)
and if the basis is not super-controlled either clamp bestNbasis to a supported
value (e.g., 1) or throw std::logic_error when approximate == false; implement
the check before calling decomp2Supercontrolled/decomp3Supercontrolled so that
decomp2Supercontrolled/decomp3Supercontrolled are only invoked when the basis
truly supports super-controlled decompositions, otherwise fallback to
decomp1(targetDecomposition) or raise an error.
| #include <cstdint> | ||
| #include <llvm/ADT/SmallVector.h> |
There was a problem hiding this comment.
Missing include for std::invalid_argument.
The function throws std::invalid_argument at Line 72 but <stdexcept> is not included.
🔧 Proposed fix
`#include` "ir/operations/OpType.hpp"
`#include` <cstdint>
`#include` <llvm/ADT/SmallVector.h>
+#include <stdexcept>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <cstdint> | |
| #include <llvm/ADT/SmallVector.h> | |
| `#include` <cstdint> | |
| `#include` <llvm/ADT/SmallVector.h> | |
| `#include` <stdexcept> |
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/EulerBasis.h` around lines 15 - 16,
Add the missing header <stdexcept> to EulerBasis.h so the throw of
std::invalid_argument used in the function that throws at the current throw site
is properly declared; update the includes at the top of
mlir/include/mlir/Passes/Decomposition/EulerBasis.h (alongside <cstdint> and
<llvm/ADT/SmallVector.h>) to include <stdexcept> to fix the compilation error
referencing std::invalid_argument.
| case EulerBasis::PSX: | ||
| [[fallthrough]]; | ||
| case EulerBasis::U1X: | ||
| break; | ||
| } | ||
| throw std::invalid_argument{ | ||
| "Unsupported euler basis for translation to gate types"}; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting why PSX and U1X are unsupported.
The PSX and U1X bases fall through to throw an exception. A brief comment explaining why these are not supported (or planned for future implementation) would help maintainability.
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/EulerBasis.h` around lines 67 - 74,
Add a brief comment above the switch/case handling the EulerBasis enum (the
cases for EulerBasis::PSX and EulerBasis::U1X that fall through to the throw
std::invalid_argument) explaining why PSX and U1X are unsupported (e.g.,
incompatible parameterization, not needed for current translation to gate types,
or planned future work) and optionally add a TODO with a reference to an
issue/PR number if implementation is intended later; place the comment next to
the cases so future readers immediately see the rationale for the thrown
exception.
| #include <Eigen/Core> | ||
| #include <array> | ||
| #include <cassert> | ||
| #include <cmath> | ||
| #include <complex> | ||
| #include <llvm/ADT/STLExtras.h> | ||
| #include <mlir/Dialect/Arith/IR/Arith.h> | ||
| #include <mlir/IR/MLIRContext.h> | ||
| #include <mlir/IR/Operation.h> | ||
| #include <mlir/IR/PatternMatch.h> | ||
| #include <mlir/IR/Value.h> | ||
| #include <mlir/IR/ValueRange.h> | ||
| #include <mlir/Support/LLVM.h> | ||
| #include <mlir/Support/LogicalResult.h> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Several includes appear unused.
Headers like <mlir/Dialect/Arith/IR/Arith.h>, <mlir/IR/MLIRContext.h>, <mlir/IR/Operation.h>, <mlir/IR/PatternMatch.h>, <mlir/IR/Value.h>, <mlir/IR/ValueRange.h>, <mlir/Support/LLVM.h>, and <mlir/Support/LogicalResult.h> do not appear to be used in this header. Additionally, <stdexcept> is needed for std::invalid_argument.
♻️ Proposed cleanup
`#include` "EulerBasis.h"
`#include` "GateSequence.h"
`#include` "Helpers.h"
`#include` "ir/Definitions.hpp"
`#include` "ir/operations/OpType.hpp"
`#include` <Eigen/Core>
`#include` <array>
-#include <cassert>
`#include` <cmath>
`#include` <complex>
-#include <llvm/ADT/STLExtras.h>
-#include <mlir/Dialect/Arith/IR/Arith.h>
-#include <mlir/IR/MLIRContext.h>
-#include <mlir/IR/Operation.h>
-#include <mlir/IR/PatternMatch.h>
-#include <mlir/IR/Value.h>
-#include <mlir/IR/ValueRange.h>
-#include <mlir/Support/LLVM.h>
-#include <mlir/Support/LogicalResult.h>
`#include` <optional>
+#include <stdexcept>🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/EulerDecomposition.h` around lines 19
- 32, The header currently includes many MLIR-specific headers that are unused
and is missing <stdexcept> needed for std::invalid_argument; remove unused
includes (specifically drop <mlir/Dialect/Arith/IR/Arith.h>,
<mlir/IR/MLIRContext.h>, <mlir/IR/Operation.h>, <mlir/IR/PatternMatch.h>,
<mlir/IR/Value.h>, <mlir/IR/ValueRange.h>, <mlir/Support/LLVM.h>, and
<mlir/Support/LogicalResult.h>) and add `#include` <stdexcept> so code using
std::invalid_argument compiles cleanly while keeping necessary headers like
<Eigen/Core>, <array>, <cassert>, <cmath>, <complex>, and
<llvm/ADT/STLExtras.h>.
| [[nodiscard]] static OneQubitGateSequence | ||
| decomposeKAK(fp theta, fp phi, fp lambda, fp phase, qc::OpType kGate, | ||
| qc::OpType aGate, bool simplify, std::optional<fp> atol) { | ||
| fp angleZeroEpsilon = atol.value_or(DEFAULT_ATOL); | ||
| if (!simplify) { | ||
| angleZeroEpsilon = -1.0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the angleZeroEpsilon = -1.0 trick.
Setting angleZeroEpsilon to -1.0 when simplify is false is clever (since std::abs(x) > -1.0 is always true), but this implicit behavior could confuse future maintainers. A brief comment would help.
📝 Suggested documentation
fp angleZeroEpsilon = atol.value_or(DEFAULT_ATOL);
if (!simplify) {
+ // Setting to -1.0 ensures all angle comparisons evaluate to true,
+ // effectively disabling simplification optimizations.
angleZeroEpsilon = -1.0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[nodiscard]] static OneQubitGateSequence | |
| decomposeKAK(fp theta, fp phi, fp lambda, fp phase, qc::OpType kGate, | |
| qc::OpType aGate, bool simplify, std::optional<fp> atol) { | |
| fp angleZeroEpsilon = atol.value_or(DEFAULT_ATOL); | |
| if (!simplify) { | |
| angleZeroEpsilon = -1.0; | |
| } | |
| [[nodiscard]] static OneQubitGateSequence | |
| decomposeKAK(fp theta, fp phi, fp lambda, fp phase, qc::OpType kGate, | |
| qc::OpType aGate, bool simplify, std::optional<fp> atol) { | |
| fp angleZeroEpsilon = atol.value_or(DEFAULT_ATOL); | |
| if (!simplify) { | |
| // Setting to -1.0 ensures all angle comparisons evaluate to true, | |
| // effectively disabling simplification optimizations. | |
| angleZeroEpsilon = -1.0; | |
| } |
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/EulerDecomposition.h` around lines 166
- 172, Document the intent of setting angleZeroEpsilon = -1.0 in decomposeKAK:
add a brief inline comment near the assignment explaining that when simplify is
false we set angleZeroEpsilon to -1.0 so subsequent comparisons like std::abs(x)
> angleZeroEpsilon always succeed (effectively disabling zero-angle clipping),
and reference DEFAULT_ATOL and the simplify flag to clarify behavior for future
maintainers.
| inline matrix2x2 getSingleQubitMatrix(const Gate& gate) { | ||
| if (gate.type == qc::SX) { | ||
| return matrix2x2{{qfp{0.5, 0.5}, qfp{0.5, -0.5}}, | ||
| {qfp{0.5, -0.5}, qfp{0.5, 0.5}}}; | ||
| } | ||
| if (gate.type == qc::RX) { | ||
| return rxMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::RY) { | ||
| return ryMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::RZ) { | ||
| return rzMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::X) { | ||
| return matrix2x2{{0, 1}, {1, 0}}; | ||
| } | ||
| if (gate.type == qc::I) { | ||
| return IDENTITY_GATE; | ||
| } | ||
| if (gate.type == qc::P) { | ||
| return pMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::U) { | ||
| return uMatrix(gate.parameter[0], gate.parameter[1], gate.parameter[2]); | ||
| } | ||
| if (gate.type == qc::U2) { | ||
| return u2Matrix(gate.parameter[0], gate.parameter[1]); | ||
| } | ||
| if (gate.type == qc::H) { | ||
| return matrix2x2{{FRAC1_SQRT2, FRAC1_SQRT2}, {FRAC1_SQRT2, -FRAC1_SQRT2}}; | ||
| } | ||
| throw std::invalid_argument{ | ||
| "unsupported gate type for single qubit matrix (" + | ||
| qc::toString(gate.type) + ")"}; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing [[nodiscard]] on getSingleQubitMatrix.
For consistency with the rest of the file, getSingleQubitMatrix should have [[nodiscard]].
♻️ Proposed fix
-inline matrix2x2 getSingleQubitMatrix(const Gate& gate) {
+[[nodiscard]] inline matrix2x2 getSingleQubitMatrix(const Gate& gate) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline matrix2x2 getSingleQubitMatrix(const Gate& gate) { | |
| if (gate.type == qc::SX) { | |
| return matrix2x2{{qfp{0.5, 0.5}, qfp{0.5, -0.5}}, | |
| {qfp{0.5, -0.5}, qfp{0.5, 0.5}}}; | |
| } | |
| if (gate.type == qc::RX) { | |
| return rxMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::RY) { | |
| return ryMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::RZ) { | |
| return rzMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::X) { | |
| return matrix2x2{{0, 1}, {1, 0}}; | |
| } | |
| if (gate.type == qc::I) { | |
| return IDENTITY_GATE; | |
| } | |
| if (gate.type == qc::P) { | |
| return pMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::U) { | |
| return uMatrix(gate.parameter[0], gate.parameter[1], gate.parameter[2]); | |
| } | |
| if (gate.type == qc::U2) { | |
| return u2Matrix(gate.parameter[0], gate.parameter[1]); | |
| } | |
| if (gate.type == qc::H) { | |
| return matrix2x2{{FRAC1_SQRT2, FRAC1_SQRT2}, {FRAC1_SQRT2, -FRAC1_SQRT2}}; | |
| } | |
| throw std::invalid_argument{ | |
| "unsupported gate type for single qubit matrix (" + | |
| qc::toString(gate.type) + ")"}; | |
| } | |
| [[nodiscard]] inline matrix2x2 getSingleQubitMatrix(const Gate& gate) { | |
| if (gate.type == qc::SX) { | |
| return matrix2x2{{qfp{0.5, 0.5}, qfp{0.5, -0.5}}, | |
| {qfp{0.5, -0.5}, qfp{0.5, 0.5}}}; | |
| } | |
| if (gate.type == qc::RX) { | |
| return rxMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::RY) { | |
| return ryMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::RZ) { | |
| return rzMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::X) { | |
| return matrix2x2{{0, 1}, {1, 0}}; | |
| } | |
| if (gate.type == qc::I) { | |
| return IDENTITY_GATE; | |
| } | |
| if (gate.type == qc::P) { | |
| return pMatrix(gate.parameter[0]); | |
| } | |
| if (gate.type == qc::U) { | |
| return uMatrix(gate.parameter[0], gate.parameter[1], gate.parameter[2]); | |
| } | |
| if (gate.type == qc::U2) { | |
| return u2Matrix(gate.parameter[0], gate.parameter[1]); | |
| } | |
| if (gate.type == qc::H) { | |
| return matrix2x2{{FRAC1_SQRT2, FRAC1_SQRT2}, {FRAC1_SQRT2, -FRAC1_SQRT2}}; | |
| } | |
| throw std::invalid_argument{ | |
| "unsupported gate type for single qubit matrix (" + | |
| qc::toString(gate.type) + ")"}; | |
| } |
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h` around lines 131 -
166, The function getSingleQubitMatrix lacks the [[nodiscard]] attribute; update
its declaration by adding [[nodiscard]] (e.g., [[nodiscard]] inline matrix2x2
getSingleQubitMatrix(const Gate& gate)) so callers are warned when the returned
matrix is ignored, keeping consistency with the rest of the file; ensure you
modify this exact function signature and no other logic.
| if (gate.type == qc::RXX) { | ||
| // TODO: check qubit order? | ||
| return rxxMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::RYY) { | ||
| // TODO: check qubit order? | ||
| return ryyMatrix(gate.parameter[0]); | ||
| } | ||
| if (gate.type == qc::RZZ) { | ||
| // TODO: check qubit order? | ||
| return rzzMatrix(gate.parameter[0]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementations of rxxMatrix, ryyMatrix, rzzMatrix
rg -t cpp "rxxMatrix|ryyMatrix|rzzMatrix" --max-count 20Repository: munich-quantum-toolkit/core
Length of output: 940
🏁 Script executed:
# Also search for the file containing these functions to understand context
fd -t f "UnitaryMatrices.h" -exec head -50 {} \;Repository: munich-quantum-toolkit/core
Length of output: 301
🏁 Script executed:
# Look for tests related to these gates and qubit ordering
find . -type f -name "*.cpp" -o -name "*.h" | xargs rg -l "RXX|RYY|RZZ" | head -20Repository: munich-quantum-toolkit/core
Length of output: 950
🏁 Script executed:
# Get the line numbers and find the full implementations
rg -n "inline matrix4x4 rxxMatrix|inline matrix4x4 ryyMatrix|inline matrix4x4 rzzMatrix" mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.hRepository: munich-quantum-toolkit/core
Length of output: 214
🏁 Script executed:
# Read the UnitaryMatrices.h file to see the implementations
cat -n mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h | grep -A 10 "inline matrix4x4 rxxMatrix"Repository: munich-quantum-toolkit/core
Length of output: 590
🏁 Script executed:
# Get more context around these functions
sed -n '1,500p' mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h | tail -150Repository: munich-quantum-toolkit/core
Length of output: 5407
🏁 Script executed:
# Look for tests that verify these matrices or qubit ordering behavior
rg -A 5 -B 5 "RXX|RYY|RZZ" mlir/unittests/decomposition/test_weyl_decomposition.cpp | head -40Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Check if there's any git history or documentation about why these TODOs were added
rg -B 5 -A 5 "TODO.*qubit order" mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.hRepository: munich-quantum-toolkit/core
Length of output: 621
🏁 Script executed:
# Verify the mathematical properties - check if the matrices are truly symmetric
# by analyzing the structure more carefully
python3 << 'EOF'
import re
# RXX matrix structure from the code:
# [0,0] = cos(θ/2)
# [0,3] = -i*sin(θ/2)
# [1,1] = cos(θ/2)
# [1,2] = -i*sin(θ/2)
# [2,1] = -i*sin(θ/2)
# [2,2] = cos(θ/2)
# [3,0] = -i*sin(θ/2)
# [3,3] = cos(θ/2)
# SWAP matrix permutes basis states: |00⟩→|00⟩, |01⟩→|10⟩, |10⟩→|01⟩, |11⟩→|11⟩
# So index mapping: 0→0, 1→2, 2→1, 3→3
# After applying SWAP*M*SWAP, element at (i,j) moves to (SWAP[i], SWAP[j])
swap_idx = {0: 0, 1: 2, 2: 1, 3: 3}
# Original RXX matrix structure
rxx_structure = {
(0, 0): "cos",
(0, 3): "-i*sin",
(1, 1): "cos",
(1, 2): "-i*sin",
(2, 1): "-i*sin",
(2, 2): "cos",
(3, 0): "-i*sin",
(3, 3): "cos"
}
# Apply SWAP transformation
transformed = {}
for (i, j), val in rxx_structure.items():
new_i = swap_idx[i]
new_j = swap_idx[j]
transformed[(new_i, new_j)] = val
print("Original RXX structure:")
for k in sorted(rxx_structure.keys()):
print(f" {k}: {rxx_structure[k]}")
print("\nAfter SWAP*M*SWAP transformation:")
for k in sorted(transformed.keys()):
print(f" {k}: {transformed[k]}")
print("\nMatrices are identical:", transformed == rxx_structure)
EOFRepository: munich-quantum-toolkit/core
Length of output: 407
🏁 Script executed:
# Check git log for the specific lines with TODOs
git log -p --follow -S "TODO: check qubit order" -- mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h | head -100Repository: munich-quantum-toolkit/core
Length of output: 53
Remove TODO comments about qubit order for RXX, RYY, RZZ gates.
RXX, RYY, RZZ are mathematically symmetric two-qubit gates where qubit order does not affect the unitary matrix. The current implementation correctly returns the same matrix regardless of qubit ordering. The TODO comments at lines 191, 195, and 199 can be safely removed.
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/UnitaryMatrices.h` around lines 190 -
201, The three TODO comments questioning qubit order for the symmetric two-qubit
gates should be removed because RXX, RYY, and RZZ are order-independent; update
the block handling gate.type == qc::RXX / qc::RYY / qc::RZZ by deleting the "//
TODO: check qubit order?" comments adjacent to the rxxMatrix(gate.parameter[0]),
ryyMatrix(gate.parameter[0]), and rzzMatrix(gate.parameter[0]) returns so the
code reads cleanly and documents that the current returns are correct.
| #include <unsupported/Eigen/MatrixFunctions> // TODO: unstable, NOLINT(misc-include-cleaner) | ||
| #include <utility> | ||
|
|
||
| namespace mlir::qco::decomposition { | ||
| /** | ||
| * Allowed deviation for internal assert statements which ensure the correctness | ||
| * of the decompositions. | ||
| */ | ||
| constexpr fp SANITY_CHECK_PRECISION = 1e-12; | ||
|
|
||
| /** | ||
| * Weyl decomposition of a 2-qubit unitary matrix (4x4). | ||
| * The result consists of four 2x2 1-qubit matrices (k1l, k2l, k1r, k2r) and | ||
| * three parameters for a canonical gate (a, b, c). The matrices can then be | ||
| * decomposed using a single-qubit decomposition into e.g. rotation gates and | ||
| * the canonical gate is RXX(-2 * a), RYY(-2 * b), RZZ (-2 * c). | ||
| */ | ||
| struct TwoQubitWeylDecomposition { | ||
| enum class Specialization : std::uint8_t { | ||
| General, // canonical gate has no special symmetry. | ||
| IdEquiv, // canonical gate is identity. | ||
| SWAPEquiv, // canonical gate is SWAP. | ||
| PartialSWAPEquiv, // canonical gate is partial SWAP. | ||
| PartialSWAPFlipEquiv, // canonical gate is flipped partial SWAP. | ||
| ControlledEquiv, // canonical gate is a controlled gate. | ||
| MirrorControlledEquiv, // canonical gate is swap + controlled gate. | ||
|
|
||
| // These next 3 gates use the definition of fSim from eq (1) in: | ||
| // https://arxiv.org/pdf/2001.08343.pdf | ||
| FSimaabEquiv, // parameters a=b & a!=c | ||
| FSimabbEquiv, // parameters a!=b & b=c | ||
| FSimabmbEquiv, // parameters a!=b!=c & -b=c | ||
| }; | ||
|
|
||
| // a, b, c are the parameters of the canonical gate (CAN) | ||
| fp a; // rotation of RXX gate in CAN (must be taken times -2.0) | ||
| fp b; // rotation of RYY gate in CAN (must be taken times -2.0) | ||
| fp c; // rotation of RZZ gate in CAN (must be taken times -2.0) | ||
| fp globalPhase; // global phase adjustment | ||
| /** | ||
| * q1 - k2r - C - k1r - | ||
| * A | ||
| * q0 - k2l - N - k1l - | ||
| */ | ||
| matrix2x2 k1l; // "left" qubit after canonical gate | ||
| matrix2x2 k2l; // "left" qubit before canonical gate | ||
| matrix2x2 k1r; // "right" qubit after canonical gate | ||
| matrix2x2 k2r; // "right" qubit before canonical gate | ||
| Specialization specialization; // detected symmetries in the matrix | ||
| EulerBasis defaultEulerBasis; // recommended euler basis for k1l/k2l/k1r/k2r | ||
| std::optional<fp> requestedFidelity; // desired fidelity; | ||
| // if set to std::nullopt, no automatic | ||
| // specialization will be applied | ||
| fp calculatedFidelity; // actual fidelity of decomposition | ||
| matrix4x4 unitaryMatrix; // original matrix for this decomposition | ||
|
|
||
| /** | ||
| * Create Weyl decomposition. | ||
| * | ||
| * @param unitaryMatrix Matrix of the two-qubit operation/series to be | ||
| * decomposed. | ||
| * @param fidelity Tolerance to assume a specialization which is used to | ||
| * reduce the number of parameters required by the canonical | ||
| * gate and thus potentially decreasing the number of basis | ||
| * gates. | ||
| */ | ||
| static TwoQubitWeylDecomposition create(const matrix4x4& unitaryMatrix, | ||
| std::optional<fp> fidelity) { | ||
| auto u = unitaryMatrix; | ||
| auto detU = u.determinant(); | ||
| auto detPow = std::pow(detU, static_cast<fp>(-0.25)); | ||
| u *= detPow; // remove global phase from unitary matrix | ||
| auto globalPhase = std::arg(detU) / 4.; | ||
|
|
||
| // this should have normalized determinant of u, so that u ∈ SU(4) | ||
| assert(std::abs(u.determinant() - 1.0) < SANITY_CHECK_PRECISION); | ||
|
|
||
| // transform unitary matrix to magic basis; this enables two properties: | ||
| // 1. if uP ∈ SO(4), V = A ⊗ B (SO(4) → SU(2) ⊗ SU(2)) | ||
| // 2. magic basis diagonalizes canonical gate, allowing calculation of | ||
| // canonical gate parameters later on | ||
| auto uP = magicBasisTransform(u, MagicBasisTransform::OutOf); | ||
| const matrix4x4 m2 = uP.transpose() * uP; | ||
|
|
||
| // diagonalization yields eigenvectors (p) and eigenvalues (d); | ||
| // p is used to calculate K1/K2 (and thus the single-qubit gates | ||
| // surrounding the canonical gate); d is is used to determine the weyl | ||
| // coordinates and thus the parameters of the canonical gate | ||
| // TODO: it may be possible to lower the precision | ||
| auto [p, d] = diagonalizeComplexSymmetric(m2, 1e-13); | ||
|
|
||
| // extract Weyl coordinates from eigenvalues, map to [0, 2*pi) | ||
| // NOLINTNEXTLINE(misc-include-cleaner) | ||
| Eigen::Vector<fp, 3> cs; | ||
| rdiagonal4x4 dReal = -1.0 * d.cwiseArg() / 2.0; | ||
| dReal(3) = -dReal(0) - dReal(1) - dReal(2); | ||
| for (int i = 0; i < static_cast<int>(cs.size()); ++i) { | ||
| assert(i < dReal.size()); | ||
| cs[i] = helpers::remEuclid((dReal(i) + dReal(3)) / 2.0, qc::TAU); | ||
| } | ||
|
|
||
| // re-order coordinates and according to min(a, pi/2 - a) with | ||
| // a = x mod pi/2 for each weyl coordinate x | ||
| decltype(cs) cstemp; | ||
| llvm::transform(cs, cstemp.begin(), [](auto&& x) { | ||
| auto tmp = helpers::remEuclid(x, qc::PI_2); | ||
| return std::min(tmp, qc::PI_2 - tmp); | ||
| }); | ||
| std::array<int, 3> order{0, 1, 2}; | ||
| llvm::stable_sort(order, | ||
| [&](auto a, auto b) { return cstemp[a] < cstemp[b]; }); | ||
| std::tie(order[0], order[1], order[2]) = | ||
| std::tuple{order[1], order[2], order[0]}; | ||
| std::tie(cs[0], cs[1], cs[2]) = | ||
| std::tuple{cs[order[0]], cs[order[1]], cs[order[2]]}; | ||
| std::tie(dReal(0), dReal(1), dReal(2)) = | ||
| std::tuple{dReal(order[0]), dReal(order[1]), dReal(order[2])}; | ||
|
|
||
| // update eigenvectors (columns of p) according to new order of | ||
| // weyl coordinates | ||
| matrix4x4 pOrig = p; | ||
| for (int i = 0; std::cmp_less(i, order.size()); ++i) { | ||
| p.col(i) = pOrig.col(order[i]); | ||
| } | ||
| // apply correction for determinant if necessary | ||
| if (p.determinant().real() < 0.0) { | ||
| auto lastColumnIndex = p.cols() - 1; | ||
| p.col(lastColumnIndex) *= -1.0; | ||
| } | ||
| assert(std::abs(p.determinant() - 1.0) < SANITY_CHECK_PRECISION); | ||
|
|
||
| // re-create complex eigenvalue matrix; this matrix contains the | ||
| // parameters of the canonical gate which is later used in the | ||
| // verification | ||
| matrix4x4 temp = dReal.asDiagonal(); | ||
| temp *= IM; | ||
| temp = temp.exp(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid unsupported Eigen/MatrixFunctions for diagonal exp.
temp is diagonal, so you can exponentiate the diagonal entries directly and drop the unsupported header.
♻️ Suggested refactor
-#include <unsupported/Eigen/MatrixFunctions> // TODO: unstable, NOLINT(misc-include-cleaner)
+// (remove unsupported/Eigen/MatrixFunctions)
matrix4x4 temp = dReal.asDiagonal();
temp *= IM;
- temp = temp.exp();
+ temp.diagonal() = temp.diagonal().array().exp().matrix();🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/WeylDecomposition.h` around lines 39 -
175, The diagonal matrix exponentiation currently uses Eigen::MatrixFunctions
(temp = temp.exp()) which relies on unsupported headers; instead compute the
complex exponentials elementwise from dReal and IM and populate temp's diagonal
directly in TwoQubitWeylDecomposition::create (use std::exp or std::polar on
each scalar complex IM * dReal(i) to set temp(i,i)), then remove the unsupported
`#include` <unsupported/Eigen/MatrixFunctions>; ensure temp remains a matrix4x4
with only the diagonal set to those exponentials and off-diagonals zero.
| } else if (newSpecialization == Specialization::SWAPEquiv) { | ||
| // :math:`U \sim U_d(\pi/4, \pi/4, \pi/4) \sim U(\pi/4, \pi/4, -\pi/4)` | ||
| // Thus, :math:`U \sim \text{SWAP}` | ||
| // | ||
| // This gate binds 0 parameters, we make it canonical by setting: | ||
| // | ||
| // :math:`K2_l = Id` , :math:`K2_r = Id`. | ||
| a = qc::PI_4; | ||
| b = qc::PI_4; | ||
| c = qc::PI_4; | ||
| if (c > 0.) { | ||
| // unmodified global phase | ||
| k1l = k1l * k2r; | ||
| k1r = k1r * k2l; | ||
| k2l = IDENTITY_GATE; | ||
| k2r = IDENTITY_GATE; | ||
| } else { | ||
| flippedFromOriginal = true; | ||
|
|
||
| globalPhase += qc::PI_2; | ||
| k1l = k1l * IPZ * k2r; | ||
| k1r = k1r * IPZ * k2l; | ||
| k2l = IDENTITY_GATE; | ||
| k2r = IDENTITY_GATE; | ||
| } |
There was a problem hiding this comment.
SWAP specialization loses the sign of c.
c is overwritten before the sign check, so the -π/4 specialization path is unreachable. If bestSpecialization() matched the negative variant, the phase/gate flips won’t be applied.
🐛 Suggested fix
- a = qc::PI_4;
- b = qc::PI_4;
- c = qc::PI_4;
- if (c > 0.) {
+ const bool negC = (c < 0.0);
+ a = qc::PI_4;
+ b = qc::PI_4;
+ c = qc::PI_4;
+ if (!negC) {
// unmodified global phase
k1l = k1l * k2r;
k1r = k1r * k2l;
k2l = IDENTITY_GATE;
k2r = IDENTITY_GATE;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (newSpecialization == Specialization::SWAPEquiv) { | |
| // :math:`U \sim U_d(\pi/4, \pi/4, \pi/4) \sim U(\pi/4, \pi/4, -\pi/4)` | |
| // Thus, :math:`U \sim \text{SWAP}` | |
| // | |
| // This gate binds 0 parameters, we make it canonical by setting: | |
| // | |
| // :math:`K2_l = Id` , :math:`K2_r = Id`. | |
| a = qc::PI_4; | |
| b = qc::PI_4; | |
| c = qc::PI_4; | |
| if (c > 0.) { | |
| // unmodified global phase | |
| k1l = k1l * k2r; | |
| k1r = k1r * k2l; | |
| k2l = IDENTITY_GATE; | |
| k2r = IDENTITY_GATE; | |
| } else { | |
| flippedFromOriginal = true; | |
| globalPhase += qc::PI_2; | |
| k1l = k1l * IPZ * k2r; | |
| k1r = k1r * IPZ * k2l; | |
| k2l = IDENTITY_GATE; | |
| k2r = IDENTITY_GATE; | |
| } | |
| } else if (newSpecialization == Specialization::SWAPEquiv) { | |
| // :math:`U \sim U_d(\pi/4, \pi/4, \pi/4) \sim U(\pi/4, \pi/4, -\pi/4)` | |
| // Thus, :math:`U \sim \text{SWAP}` | |
| // | |
| // This gate binds 0 parameters, we make it canonical by setting: | |
| // | |
| // :math:`K2_l = Id` , :math:`K2_r = Id`. | |
| const bool negC = (c < 0.0); | |
| a = qc::PI_4; | |
| b = qc::PI_4; | |
| c = qc::PI_4; | |
| if (!negC) { | |
| // unmodified global phase | |
| k1l = k1l * k2r; | |
| k1r = k1r * k2l; | |
| k2l = IDENTITY_GATE; | |
| k2r = IDENTITY_GATE; | |
| } else { | |
| flippedFromOriginal = true; | |
| globalPhase += qc::PI_2; | |
| k1l = k1l * IPZ * k2r; | |
| k1r = k1r * IPZ * k2l; | |
| k2l = IDENTITY_GATE; | |
| k2r = IDENTITY_GATE; | |
| } |
🤖 Prompt for AI Agents
In `@mlir/include/mlir/Passes/Decomposition/WeylDecomposition.h` around lines 612
- 636, The code in the Specialization::SWAPEquiv branch overwrites c to qc::PI_4
before testing its sign, so the negative-specialization path can never run; fix
it by preserving the original c sign (e.g., store double originalC = c; before
setting a=b=c = qc::PI_4) and then use originalC > 0. rather than c > 0. when
deciding whether to apply the flipped/global-phase adjustments to k1l, k1r,
globalPhase and k2l/k2r; refer to newSpecialization/Specialization::SWAPEquiv,
variables a,b,c and the gate/phase updates to k1l,k1r,k2l,k2r and globalPhase
(or alternately detect the negative variant from bestSpecialization() and branch
accordingly).
| addOptimizationPasses(pm); | ||
| addCleanupPasses(pm); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid running cleanup twice around the optimization stage.
Stage 5 already calls addCleanupPasses before recording afterOptimization, then Stage 6 runs addCleanupPasses again. This makes afterOptimizationCanon redundant and adds extra pass cost. Consider moving cleanup exclusively to Stage 6.
♻️ Suggested change
// Stage 5: Optimization passes
addOptimizationPasses(pm);
-addCleanupPasses(pm);
if (failed(pm.run(module))) {
return failure();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addOptimizationPasses(pm); | |
| addCleanupPasses(pm); | |
| addOptimizationPasses(pm); |
🤖 Prompt for AI Agents
In `@mlir/lib/Compiler/CompilerPipeline.cpp` around lines 169 - 170, The cleanup
passes are being run twice: addCleanupPasses(pm) is invoked before recording
afterOptimization (making afterOptimizationCanon redundant) and then again in
Stage 6; remove the first addCleanupPasses call so cleanup is only applied in
Stage 6, or alternatively stop recording afterOptimizationCanon and only record
afterOptimization after Stage 6. Update the code around
addOptimizationPasses(pm) / addCleanupPasses(pm) and the recording of
afterOptimization/afterOptimizationCanon so that addCleanupPasses is invoked
exactly once (refer to addOptimizationPasses, addCleanupPasses,
afterOptimization, and afterOptimizationCanon to locate and adjust the logic).
3 participants
Description
TODO
Related to #1122
Checklist: