✨ Add QCO Euler synthesis and fuse-single-qubit-unitary-runs pass#1672
✨ Add QCO Euler synthesis and fuse-single-qubit-unitary-runs pass#1672simon1hofmann wants to merge 29 commits into
fuse-single-qubit-unitary-runs pass#1672Conversation
simon1hofmann
added
feature
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new UnitaryMatrixOpInterface and attaches it to QCO ops; implements Euler-angle extraction and synthesis across several bases; adds the fuse-single-qubit-unitary-runs pass that composes and resynthesizes contiguous 1-qubit matrix-backed gates; updates wiring, utilities, and tests to exercise fusion and synthesis. ChangesEuler Decomposition, Unitary Matrix Interface, and Single-Qubit Fusion
Sequence Diagram(s)sequenceDiagram
participant Pass as FuseSingleQubitUnitaryRuns
participant Collector as RunCollector
participant Composer as MatrixComposer
participant Synth as EulerSynthesizer
participant IR as MLIR
Pass->>Collector: register basis-configured rewrite pattern
Collector->>Collector: find maximal contiguous 1Q matrix-backed runs
Collector->>Composer: extract per-op 2x2 matrices (execution order)
Composer->>Composer: compose run matrix (left-multiply)
Composer->>Synth: synthesizeUnitary1QEuler(composed matrix, basis)
Synth->>IR: emit synthesized ops (basis gates ± gphase)
Pass->>IR: replace uses and erase original run ops
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)mlir/lib/Dialect/QCO/Utils/WireIterator.cppmlir/lib/Dialect/QCO/Utils/WireIterator.cpp:11:10: fatal error: 'mlir/Dialect/QCO/Utils/WireIterator.h' file not found ... [truncated 1180 characters] ... /include" mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp:11:10: fatal error: 'mlir/Dialect/QCO/Builder/QCOProgramBuilder.h' file not found ... [truncated 2200 characters] ... ns.ml", line 4782, characters 12-47 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.
| 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)?
There was a problem hiding this comment.
Probably better as a separate fold than a replacement here: this mod2pi runs during derivation (before any ops exist), and the wrap is entangled with the phase correction, so an op fold can't subsume it.
A general fold that normalizes constant rotation angles into [-pi, pi) with a compensating gphase would be a nice reusable cleanup though, maybe as a follow-up?
There was a problem hiding this comment.
You are probably right.
Even though the global phase is also 2pi periodic, so the phase correction would probably still be correct even if the normalization would not be performed here.
Still probably better to leave this as-is for now.
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.
✅ Actions performedFull review triggered. |
|
@burgholzer The implementation changed quite a lot, would be great to get another review. |
I'll try to get to this asap. |
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @simon1hofmann for the iteration on this. We are definitely moving in the right direction. This already feels much better.
I still have quite a couple of comments left; most importantly on the performance of the decomposition pass. But it feels like we are converging and I like where this is heading! Keep it up 🙌🏼
There was a problem hiding this comment.
Just to question the added value of the separate header a little bit and because I am curious: Does this really bring us a meaningful advantage in terms of not having to include Eigen (transitively) as often? How often do we only import the QCODialect or QCOInterfaces header, but not the QCOOps header?
There was a problem hiding this comment.
Good question — I let Opus 4.8 dug into the actual include graph: TL;DR: the split buys us no measurable Eigen reduction today.
Where Eigen actually enters
Only one header pulls Eigen, and QCOOps.h hard-includes it:
QCODialect.h→ no Eigen, doesn't reference the interfaces at allQCOInterfaces.h→ no Eigen (structural interfaces only)QCOUnitaryMatrixInterfaces.h→#include <Eigen/Core>
So Eigen reaches a TU iff it pullsQCOUnitaryMatrixInterfaces.h, directly or viaQCOOps.h.
How often is a header used without QCOOps.h?
Enumerating every includer across mlir/ (lib/, unittests/, tools/, headers):
| Header | Total includers | …that do not also include QCOOps.h |
|---|---|---|
QCOInterfaces.h |
7 TUs (+2 headers) | 0 |
QCODialect.h |
~32 | ~14 |
The decisive row is QCOInterfaces.h: every consumer also includes QCOOps.h (Drivers.h, FuseSingleQubitUnitaryRuns.cpp, HadamardLifting.cpp, MergeSingleQubitRotationGates.cpp, WireIterator.cpp, plus two tests). Since QCOOps.h unconditionally drags in QCOUnitaryMatrixInterfaces.h, splitting Eigen out of QCOInterfaces.h removes Eigen from zero translation units.
The QCODialect.h-only consumers (~14) never carried Eigen anyway, since QCODialect.h doesn't touch the matrix interface — so the split is irrelevant to them too.
Verdict
No, the separate header doesn't reduce transitive Eigen inclusion in practice. There's no TU that wants QCOInterfaces.h/QCODialect.h but can avoid QCOOps.h, and QCOOps.h must pull the Eigen-typed UnitaryMatrixOpInterface because the generated op classes implement it. Net effect: "use a QCO op ⇒ get Eigen."
What the split does give us (non-Eigen, worth keeping):
- Keeps
QCOInterfaces.hand its generated.h.incEigen-free, cleanly separating the structural interface (UnitaryOpInterface: wire/target/isSingleQubit) from the matrix interface. - Leaves the door open for a future structure-only consumer to stay Eigen-free — latent until such a consumer exists.
If the real goal is to cut Eigen's compile-time footprint, the lever isn't this split — it's making UnitaryMatrixOpInterface not expose Eigen types in its public signature (e.g. std::array<std::complex<double>, 4> at the boundary, Eigen confined to the .cpp). That would let QCOOps.h drop <Eigen/Core> entirely, which is where the actual cost is.
There was a problem hiding this comment.
Hm. So the conclusion is that the split did not bring any advantages.. unfortunate.
We may want to investigate whether there are alternatives to this. It feels wrong to me to incur the include penalty on every translation unit even though very little passes will actually need the matrices.
There was a problem hiding this comment.
I did not particularly check this file in all of its detail yet. Will do eventually.
- Updated the EulerBasis enum to remove ZSX and clarify ZSXX description. - Simplified function signatures in Euler.cpp and related files by removing the 'simplify' parameter. - Adjusted error messages and test cases to reflect the changes in Euler basis options. - Cleaned up unnecessary code and comments for better readability.
- Removed unnecessary constant creation functions and replaced them with direct calls to operation constructors for RZ, RY, RX, U, SX, and X operations. - Updated the `accumulateEmittedMatrix` function to accept an interface directly, simplifying the code. - Renamed the `emitRzAsP` function to `emitCanonicalRZ` for clarity and consistency. - Cleaned up code for better readability and maintainability.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 61-62: Passes.td defines the Option list for "basis" but the help
string omits the newly supported "zsx"; update the option definition (the
options variable with Option<"basis", ...>) to include "zsx" in the quoted list
of supported bases (e.g., add , "zsx" into the help string) and also update the
invalid-basis error message in FuseSingleQubitUnitaryRuns.cpp (the code path
that validates/prints supported bases) to include "zsx" so the public help and
runtime validation/errors consistently list the new basis.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 349-351: CtrlOp::verify() can reach cases where bodyUnitary
implements UnitaryOpInterface but not UnitaryMatrixOpInterface (e.g., ctrl {
barrier }), so replace the unchecked cast to UnitaryMatrixOpInterface on
bodyUnitary.getOperation() and the direct use of
getUnitaryMatrix<Eigen::MatrixXcd>() with a safe test: check
dyn_cast<UnitaryMatrixOpInterface>(bodyUnitary.getOperation()) and if it returns
null, return llvm::None (nullopt) from the function/path that expects an absent
matrix; only call getUnitaryMatrix when the dyn_cast succeeds. Ensure you update
any callers of this code path to handle the nullopt accordingly.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp`:
- Around line 410-412: InvOp::getUnitaryMatrix() currently performs an
unconditional cast on bodyUnitary.getOperation() to UnitaryMatrixOpInterface and
crashes for inner ops that are not matrix-backed (e.g., inv { barrier }); change
this to first check that bodyUnitary.getOperation() implements
UnitaryMatrixOpInterface (using isa/dyn_cast or
op.getInterface<UnitaryMatrixOpInterface>()) and if the check fails return “no
matrix” (the same sentinel your API uses) instead of casting; only call
getUnitaryMatrix<Eigen::MatrixXcd>() after the successful interface check so
non-matrix inner ops are safely guarded.
In
`@mlir/lib/Dialect/QCO/Transforms/NativeSynthesis/FuseSingleQubitUnitaryRuns.cpp`:
- Around line 95-110: wireStarts is currently seeded only with
AllocOp/StaticOp/qtensor::ExtractOp results and func::FuncOp entry args, so
block arguments of nested regions (loops/ifs) are missed; update the seeding to
also walk region-bearing ops (e.g. scf.for, scf.while, qco.if / scf.if) and for
each op inspect its region entry block(s) and emplace any BlockArgument whose
type isa<QubitType> into wireStarts (or alternatively replace the global seed
approach with a block-local traversal that starts from every block argument of
every region); refer to the existing module.walk usage and the wireStarts vector
when adding these additional seeds.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 102-107: The basis list in function forEachBasis omits "zsx", so
update the const std::array named bases in forEachBasis to include the missing
"zsx" token (e.g., add "zsx" into the initializer alongside "zsxx" etc.) so the
randomized synthesis, fusion, and boundary tests exercise the advertised basis;
modify the bases array in forEachBasis accordingly.
- Around line 514-545: The tests runFuseOnProgramForAllBases currently only
check matrices and boundary op count but not that the fuse pass preserved the
structural split; update both tests
(FuseSingleQubitUnitaryRunsTest::DoesNotFuseAcrossTwoQGateAllBases and
::DoesNotFuseAcrossBarrierAllBases) to locate the first boundary operation
(identify via singleQubitRunsSplitByTwoQGate's TwoQ gate / BarrierOp
respectively using countTwoQubitGates or countOps<BarrierOp> as markers), then
scan the function body (func::FuncOp funcOp) to count one‑qubit unitary ops
before and after that first boundary op (e.g., iterate ops in the entry block
and classify using your existing one‑qubit gate predicates or by matching the
basis gate ops used by expectBasisGatesOnly), and add EXPECT_GE(count_before,
1U) and EXPECT_GE(count_after, 1U) assertions to ensure at least one 1Q unitary
remains on each side of the boundary.
🪄 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: 67455fad-b33d-451e-9f90-232332319998
📒 Files selected for processing (22)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/IR/CMakeLists.txtmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.hmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.hmlir/include/mlir/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.tdmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Euler.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Dialect/QCO/IR/CMakeLists.txtmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.cppmlir/lib/Dialect/QCO/Transforms/CMakeLists.txtmlir/lib/Dialect/QCO/Transforms/Decomposition/Euler.cppmlir/lib/Dialect/QCO/Transforms/NativeSynthesis/FuseSingleQubitUnitaryRuns.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cppmlir/unittests/programs/qco_programs.h
💤 Files with no reviewable changes (2)
- mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.h
- mlir/unittests/programs/qco_programs.h
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/lib/Dialect/QCO/Utils/WireIterator.cpp`:
- Around line 47-48: WireIterator::qubit() currently treats only SinkOp and
qtensor::InsertOp as boundary ops, causing inconsistency with other iterator
methods that also treat YieldOp and scf::YieldOp as boundaries; update
WireIterator::qubit() so it checks for isa<SinkOp, YieldOp, qtensor::InsertOp,
scf::YieldOp>(op_) (or otherwise include YieldOp and scf::YieldOp in its
boundary check) and return nullptr / set isSentinel_ the same way as the other
methods (keep the same sentinel behavior used where isSentinel_ is set).
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 205-240: compute1QMatrixFromFunction only inspects the entry block
and misses ops nested in regions (e.g. scf.for bodies), so tests like
FusesRunInScfForBody compare identity-to-identity; modify
compute1QMatrixFromFunction to traverse all blocks in the function (or
recursively walk regions) rather than only using funcOp.getBody().front():
iterate every block/operation in the function body (including nested regions and
scf.for bodies), keep the existing filtering logic (skip arith::ConstantOp and
isTwoQubitGate), and apply the GPhaseOp and UnitaryMatrixOpInterface handling to
operations found in nested blocks so the accumulated acc/global reflect
loop-body gates too.
🪄 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: faaec2e4-f989-41ee-8ab4-d3f91ffb73cf
📒 Files selected for processing (24)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/IR/CMakeLists.txtmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.hmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.hmlir/include/mlir/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.tdmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Euler.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Dialect/Utils/Utils.hmlir/lib/Dialect/QCO/IR/CMakeLists.txtmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOUnitaryMatrixInterfaces.cppmlir/lib/Dialect/QCO/Transforms/CMakeLists.txtmlir/lib/Dialect/QCO/Transforms/Decomposition/Euler.cppmlir/lib/Dialect/QCO/Transforms/NativeSynthesis/FuseSingleQubitUnitaryRuns.cppmlir/lib/Dialect/QCO/Utils/CMakeLists.txtmlir/lib/Dialect/QCO/Utils/WireIterator.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cppmlir/unittests/programs/qco_programs.h
💤 Files with no reviewable changes (2)
- mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.h
- mlir/unittests/programs/qco_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Description
QCOUnitaryMatrixInterfacesfromQCOInterfaces.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.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.