✨ Add QCO Euler synthesis and fuse-single-qubit-unitary-runs pass#1672
Conversation
simon1hofmann
added
feature
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Euler-angle extraction and synthesis APIs supporting six bases (ZYZ/ZXZ/XZX/XYX/U/ZSXX), a basis-configurable ChangesEuler Decomposition and Single-Qubit Unitary Fusion
Sequence Diagram(s)sequenceDiagram
participant PassMgr as PassManager
participant FusePass as FuseSingleQubitUnitaryRunsPass
participant Pattern as FusePattern
participant WireIter as WireIterator
participant Synth as synthesizeUnitary1QEuler
PassMgr->>FusePass: runOnOperation(ModuleOp)
FusePass->>Pattern: register pattern with basis
FusePass->>Pattern: applyPatternsGreedily
Pattern->>WireIter: scanFusableRun from head
WireIter-->>Pattern: contiguous ops + composed 2x2
Pattern->>Synth: composed matrix + basis
Synth-->>Pattern: emitted basis ops (or nullopt)
Pattern-->>PassMgr: replace uses + erase originals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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: 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.
Split matrix extraction from UnitaryOpInterface so gates can expose constant unitary matrices only when parameters are known at compile time.
Replace the old GateSequence/Helpers/UnitaryMatrices split with a single Euler API (angle extraction + IR synthesis) and unify MLIR numeric tolerance.
Fuse maximal runs of constant 1Q unitaries on a wire into basis-native gates, with configurable Euler basis and exact global-phase correction via gphase.
…fixture, and enhance matrix functions for clarity and consistency
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@burgholzer I went through your comments today and also found some more optimization/refactoring potential. |
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
…e in range-based constructs Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Alright. I spent a couple of hours working through this myself and simplified several bits and pieces across the implementation. Also optimized a few corner cases in the synthesis itself that should now, hopefully result in better decompositions (mostly cases with theta==0 where gates can directly be fused).
From my side, this feels good to go now, if CI is green. 🙌🏼
Please have a look through the individual commits that I pushed to think through the changes and check whether they make sense.
If they do, feel free to merge the PR at your convenience.
If not, or you would like to improve some things further, don't hesitate to go for it. 🚀
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Thanks a lot for the additional time you spent on this, I went through your commits and they all make sense to me. |
Alright, then let's tag this one for auto-merge 🙌🏼 🚀 |
3 participants
Description
Euler.h/Euler.cpp(angle extraction +synthesizeUnitary1QEulerIR emission), replacing the previousEulerDecomposition/GateSequence/Helpers/UnitaryMatricessplit.fuse-single-qubit-unitary-runsMLIR pass: fuse maximal runs of constant single-qubit unitaries on a wire by composing 2×2 matrices and resynthesizing in a configurable Euler basis (zyz,zxz,xzx,xyx,u,zsxx), withgphasefor exact global phase.Split up and refactored from #1665.
AI Assistance
Used Composer via Cursor for parts of this change. I reviewed the full diff and take responsibility for everything in this PR.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.