✨ Add Euler decomposition and supporting decomposition primitives#1672
✨ Add Euler decomposition and supporting decomposition primitives#1672simon1hofmann wants to merge 7 commits intomainfrom
Conversation
simon1hofmann
added
feature
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new Euler decomposition subsystem to the MLIR QCO dialect: gate/sequence abstractions, Euler-basis enums and mapping, algorithms to factor 2×2 unitaries into basis-specific gate sequences with exact global-phase accounting, matrix utilities, helpers, and extensive unit tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant EulerDecomposition
participant AngleExtraction
participant GateEmitter
participant GateSequence
User->>EulerDecomposition: generateCircuit(basis, unitary, simplify, atol)
activate EulerDecomposition
EulerDecomposition->>AngleExtraction: anglesFromUnitary(unitary, basis)
activate AngleExtraction
AngleExtraction->>AngleExtraction: paramsZyz/Zxz/Xyx/Xzx/U1x(unitary)
AngleExtraction-->>EulerDecomposition: (theta, phi, lambda, phase)
deactivate AngleExtraction
alt Basis is Z-Y-Z / Z-X-Z / X-Y-X / X-Z-X
EulerDecomposition->>GateEmitter: decomposeKAK(angles, kGate, aGate, simplify, atol)
else Basis is ZSX / ZSXX
EulerDecomposition->>GateEmitter: decomposePsxGen(angles, allowXShortcut, simplify, atol)
else Basis is U / U321 / U3
EulerDecomposition->>GateEmitter: emit single U gate
end
activate GateEmitter
GateEmitter->>GateSequence: add Gate(type, params, qubits)
GateEmitter->>GateSequence: set globalPhase
GateEmitter-->>EulerDecomposition: OneQubitGateSequence
deactivate GateEmitter
EulerDecomposition-->>User: OneQubitGateSequence
deactivate EulerDecomposition
User->>GateSequence: getUnitaryMatrix()
activate GateSequence
GateSequence->>GateSequence: multiply gate matrices in order
GateSequence->>GateSequence: apply globalPhaseFactor
GateSequence-->>User: Eigen::Matrix4cd
deactivate GateSequence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 29 seconds.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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.h`:
- Around line 28-32: isUnitaryMatrix currently only checks
(matrix.transpose().conjugate() * matrix).isIdentity(...) which allows
rectangular matrices with orthonormal columns to pass; modify isUnitaryMatrix to
first confirm the matrix is square (matrix.rows() == matrix.cols()) and return
false if not, then perform the unitary check (using adjoint if preferred) so
only true square unitary matrices are accepted; reference isUnitaryMatrix and
the matrix.transpose().conjugate() * matrix identity check to locate where to
add the square check.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cpp`:
- Around line 79-87: The ZYZ branch currently returns the raw phase from
paramsZyz(matrix) which exposes γ; to preserve the existing
anglesFromUnitary(..., EulerBasis::ZYZ) contract you must absorb the qco.u
intrinsic offset into the returned phase: call paramsZyz(matrix) to get the
decomposition, then adjust its phase by adding (phi + lambda) / 2 (i.e., phase
+= (phi + lambda)/2) before returning; reference EulerBasis::ZYZ, paramsZyz, and
anglesFromUnitary to locate and apply this change.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cpp`:
- Around line 48-57: The getComplexity function incorrectly applies the
multi-qubit cost before checking for decomposition::GateKind::GPhase, causing
getComplexity(GPhase, n>1) to return a positive cost; change the logic in
getComplexity so that GateKind::GPhase is handled first (or otherwise
short-circuited) and always returns 0 regardless of numOfQubits—update the
function around the multi-qubit branch to check for
decomposition::GateKind::GPhase before applying the multiQubitFactor or add an
explicit early return for GPhase.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cpp`:
- Around line 35-42: Add a regression assertion to the existing
TEST(DecompositionHelpersTest, GetComplexitySingleQubitAndGphase) that verifies
getComplexity(GateKind::GPhase, N) returns 0 for multi-qubit inputs (e.g., call
getComplexity(GateKind::GPhase, 2) and assert EXPECT_EQ(..., 0U)); this ensures
the zero-cost invariant for GateKind::GPhase holds when numOfQubits > 1.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 118-123: The test currently reconstructs only
u3Matrix(angles[0],angles[1],angles[2]) and uses isEquivalentUpToGlobalPhase,
which ignores angles[3]; update the test to include the extracted phase from
EulerDecomposition::anglesFromUnitary (angles[3]) by multiplying u3Matrix(...)
by exp(i*angles[3]) and assert that this phased reconstruction equals the
original hadamard (using a direct matrix equality/tolerance check rather than
isEquivalentUpToGlobalPhase) so the returned phase is validated for the
SingleQubitMode::U3 contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60ac46cf-2573-4949-b882-448254fc15d0
📒 Files selected for processing (18)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerBasis.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Gate.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateKind.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateSequence.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.hmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerBasis.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/GateSequence.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cppmlir/unittests/TestCaseUtils.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Tamino Bauknecht <dev@tb6.eu>
simon1hofmann
force-pushed
the
decomp/euler
branch
from
5b752b4 to
e5280c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.h`:
- Around line 24-49: randomUnitaryMatrix currently only enforces fixed-size
matrices but can accept non-square fixed-size types; add a compile-time check to
require square matrices by adding a static_assert that
MatrixType::RowsAtCompileTime == MatrixType::ColsAtCompileTime (with a clear
message like "randomUnitaryMatrix requires square matrices") near the existing
fixed-size static_assert in the randomUnitaryMatrix template so misuse with
rectangular fixed-size MatrixType is prevented; keep the existing use of
MatrixType and dim as-is.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 173-180: The test for ZSXX collapsing to a bare X should assert
there are no other gate kinds or extra gates: after calling
EulerDecomposition::generateCircuit(EulerBasis::ZSXX, pauliX, true,
std::nullopt) and the existing EXPECT_EQ(countGatesOfType(seq, GateKind::X), 1U)
and sequenceMatchesSingleQubitMatrix check, also assert the total gate count
equals 1 (e.g., using countTotalGates(seq) or summing known GateKind counts)
and/or assert that the set of gate kinds present is exactly {GateKind::X} (e.g.,
ensure countGatesOfType(seq, GateKind::RZ)==0 and similarly zero for SX, H,
etc.) so no RZ/SX/other gates accompany the X; reference the TEST name
EulerDecompositionTest::ZsxxPauliXUsesSingleXGate,
EulerDecomposition::generateCircuit, countGatesOfType, GateKind, and
sequenceMatchesSingleQubitMatrix when adding these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cce6649a-1227-417d-8c76-5eead11f9fca
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cpp`:
- Around line 163-165: The current code silently returns
Eigen::Matrix4cd::Identity() when gate.qubitId.empty(), which masks malformed
input; instead, validate qubitId explicitly and fail fast: if gate.qubitId is
empty and gate.kind is not the explicit identity kind, emit a clear error or
assertion (e.g., llvm::report_fatal_error or assert) rather than returning an
identity matrix; only allow returning Eigen::Matrix4cd::Identity() when
gate.kind is the identity gate and qubitId legitimately indicates an identity
operation. Locate the branch using gate.qubitId and Eigen::Matrix4cd::Identity()
and replace the unconditional identity fallback with the described input check
and failure.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 90-110: The test TEST(EulerDecompositionTest, Random) uses
maxIterations = 10000 which is too slow for regular unit tests; change this test
to use a much smaller deterministic iteration count (e.g., 100) by reducing
maxIterations and keeping the same rng seed and logic, and move the heavy loop
into a new separate stress test (e.g., TEST(EulerDecompositionStress,
RandomStress)) that runs 10000 iterations; ensure the new stress test uses the
same calls (randomUnitaryMatrix, EulerDecomposition::generateCircuit,
EulerDecompositionTest::restore) and preserves the existing EXPECT_TRUE/isApprox
checks so CI unit tests stay fast while stress runs still validate the full
randomized workload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3dacb7d-3453-4bde-8bba-2f70a91add21
📒 Files selected for processing (17)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerBasis.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Gate.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateKind.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateSequence.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.hmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerBasis.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/GateSequence.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp
Co-authored-by: Copilot <copilot@github.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @simon1hofmann for kickstarting this endeavor!
I finally found some time to think a little bit about the changes coming up here. You'll find a lot of my thoughts in the inline comments.
Some of these comments are probably not too relevant because I am just missing the complete picture. However, I believe there's a general trend to the comments: I have a bit of an aversion of introducing too much dedicated infrastructure for the decomposition and would rather like to build this more MLIR-like.
I would have hoped that passes like single-qubit decomposition can be rather easily implemented as transformation passes that directly operate on the QCO IR; potentially in a two-step process that traverses the circuit once to collect runs of single-qubit gates and merges their matrices and a second pass that decomposes to the right gate-set. The first pass would leave sequences of single gates alone whenever they are already in the basis.
There are a couple of other comments sprinkled across the files that are probably worth discussing. Hopefully, this gives you a good sense though where I would like this to go eventually.
| inline constexpr double FRAC1_SQRT2 = | ||
| 0.707106781186547524400844362104849039284835937688474036588L; | ||
|
|
||
| /// Generic 3-parameter single-qubit unitary `U(theta, phi, lambda)`. | ||
| [[nodiscard]] Eigen::Matrix2cd uMatrix(double theta, double phi, double lambda); | ||
| /// `U2(phi, lambda) == U(pi/2, phi, lambda)`. | ||
| [[nodiscard]] Eigen::Matrix2cd u2Matrix(double phi, double lambda); | ||
| /// Axis rotations `exp(-i theta/2 * sigma_{x,y,z})`. | ||
| [[nodiscard]] Eigen::Matrix2cd rxMatrix(double theta); | ||
| [[nodiscard]] Eigen::Matrix2cd ryMatrix(double theta); | ||
| [[nodiscard]] Eigen::Matrix2cd rzMatrix(double theta); | ||
| /// Two-qubit Ising-style rotations on the `XX`, `YY`, `ZZ` generators. | ||
| [[nodiscard]] Eigen::Matrix4cd rxxMatrix(double theta); | ||
| [[nodiscard]] Eigen::Matrix4cd ryyMatrix(double theta); | ||
| [[nodiscard]] Eigen::Matrix4cd rzzMatrix(double theta); | ||
| /// Phase gate `diag(1, exp(i lambda))`. | ||
| [[nodiscard]] Eigen::Matrix2cd pMatrix(double lambda); | ||
|
|
||
| inline const Eigen::Matrix2cd H_GATE{{FRAC1_SQRT2, FRAC1_SQRT2}, | ||
| {FRAC1_SQRT2, -FRAC1_SQRT2}}; |
There was a problem hiding this comment.
None of these matrices shall need to be defined here as they are already defined as part of the respective operations. We should try to avoid as much redundancy in the code base as we can.
However, one thought that I already had a while back: It is kind of inconvenient that the QCO UnitaryOpInterface has a public dependency on Eigen and that this dependency is propagated literally everywhere, even though the respective code is hardly needed anywhere so far.
We could/should consider defining a dedicated UnitaryMatrixInterface that is defined separately from the UnitaryOpInterface and provides all the related methods.
We should make sure that that interface contains all the necessary methods so that none of the helpers here are necessary.
| /** | ||
| * Lightweight gate identifiers used by decomposition utilities. | ||
| * | ||
| * These kinds intentionally stay independent from the core IR `qc::OpType` | ||
| * enum so the MLIR/QCO decomposition layer does not depend on the `ir` | ||
| * package. | ||
| */ | ||
| enum class GateKind : std::uint8_t { | ||
| I = 0, | ||
| H, | ||
| P, | ||
| U, | ||
| U2, | ||
| X, | ||
| Y, | ||
| Z, | ||
| SX, | ||
| RX, | ||
| RY, | ||
| RZ, | ||
| R, | ||
| RXX, | ||
| RYY, | ||
| RZZ, | ||
| GPhase, | ||
| }; |
There was a problem hiding this comment.
I would hope that we do not need this kind of enumeration as we can simply reuse the type information of the MLIR operations. I'd really like to avoid adding too many separate enumerations and classes for concepts that are already well represented in the IR.
| /** | ||
| * Lightweight decomposition-time gate record. | ||
| * | ||
| * This struct is intentionally independent from MLIR operations so helper code | ||
| * can build and manipulate abstract one- and two-qubit circuits before they | ||
| * are materialized back into the IR. | ||
| */ | ||
| struct Gate { | ||
| /// Operation kind represented by this gate. | ||
| GateKind type{GateKind::I}; | ||
|
|
||
| /// Gate parameters in operation-specific order. | ||
| SmallVector<double> parameter; | ||
|
|
||
| /// Logical qubit ids used by the gate, in operand order. | ||
| SmallVector<QubitId> qubitId = {0}; | ||
| }; |
There was a problem hiding this comment.
I would have hoped that we can implement the decomposition logic itself without adding a dedicated class that duplicates information.
At least for the single-qubit decomposition I don't really see why this would be necessary.
Is this really so much more compact than the respective gate operations?
| unitaryMatrix = gateMatrix * unitaryMatrix; | ||
| } | ||
| unitaryMatrix *= helpers::globalPhaseFactor(globalPhase); | ||
| assert(helpers::isUnitaryMatrix(unitaryMatrix)); |
There was a problem hiding this comment.
Given how we are only ever multiplying unitary matrices I believe this is a given and can be removed in general.
| return {GateKind::RZ, GateKind::SX, GateKind::X}; | ||
| } | ||
| llvm::reportFatalInternalError( | ||
| "Unsupported euler basis for translation to gate types"); |
There was a problem hiding this comment.
"Euler" should always be capitalized
| /** | ||
| * Decompose a 2x2 unitary into the gate alphabet described by | ||
| * `targetBasis`. | ||
| * | ||
| * When `simplify` is true, near-zero angles are removed using `atol` (or | ||
| * `DEFAULT_ATOL` if no override is provided). The returned global phase keeps | ||
| * the decomposition exactly equal to `unitaryMatrix`. | ||
| */ | ||
| [[nodiscard]] static OneQubitGateSequence | ||
| generateCircuit(EulerBasis targetBasis, const Eigen::Matrix2cd& unitaryMatrix, | ||
| bool simplify, std::optional<double> atol); |
There was a problem hiding this comment.
I can't help but feel that this should be a pattern rewrite rule. This doesn't really feel "MLIR" yet.
| private: | ||
| /// Extract parameters for a `rz(phi) · ry(theta) · rz(lambda)` factorization. | ||
| [[nodiscard]] static std::array<double, 4> | ||
| paramsZyz(const Eigen::Matrix2cd& matrix); |
There was a problem hiding this comment.
Should functions like these be stylized as paramsZYZ? The current capitalization looks unusual and a bit odd.
| [[nodiscard]] static OneQubitGateSequence | ||
| decomposePsxGen(double theta, double phi, double lambda, double phase, | ||
| bool allowXShortcut, bool simplify, | ||
| std::optional<double> atol); |
There was a problem hiding this comment.
In other parts of the library, the tolerance is passed as a double with a suitable default. The interfaces should be consistent across the library. I'd have a personal preference for avoiding optional here.
| // Qiskit's `params_u1x_inner` reuses Z-Y-Z angles but shifts the global | ||
| // phase by `-0.5 * (theta + phi + lambda)` so that the decomposition | ||
| // matches an `rz`/`sx` emission exactly (not only up to global phase). |
There was a problem hiding this comment.
Do we really need to refer to Qiskit here and cannot present this as a factual comment?
We can also deliberately use different naming if we feel like that is more suitable.
| auto newPhi = helpers::mod2pi(phi + std::numbers::pi, 0.); | ||
| auto newLam = helpers::mod2pi(lam + std::numbers::pi, 0.); | ||
| return { |
There was a problem hiding this comment.
Would it make sense to somewhat implement this normalization of the angles direcly on the operations themselves in MLIR (e.g. as folds)?
3 participants
Description
Split up from #1665 implementing Euler decomposition.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.