✨ Add Rotation Gate Merging using Quaternions#1407
Open
J4MMlE wants to merge 31 commits intomunich-quantum-toolkit:mainfrom
Open
✨ Add Rotation Gate Merging using Quaternions#1407J4MMlE wants to merge 31 commits intomunich-quantum-toolkit:mainfrom
J4MMlE wants to merge 31 commits intomunich-quantum-toolkit:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
burgholzer
added
feature
Member
|
Hey @J4MMlE 👋🏻 How much of an ask would it be to directly base this pass on the QCO dialect and its infrastructure? |
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
106575c to
7528b05
Compare
DRovara
requested changes
Jan 20, 2026
Collaborator
There was a problem hiding this comment.
Thanks a lot @J4MMlE for the effort! The code already looks super clean in my opinion and I like the way you have implemented everything.
I do have quite a few minor comments, but they are largely just related to comments in the code (docstrings/typos).
What I did not look at in much detail is the CMake setup. I guess it would make sense if @burgholzer (after your vacation, of course) could look into that - although probably it would be most efficient to do that only once the tests are ready, too (btw, I think the point from my top-level comment below might also be interesting to consider for you).
Anyways, @J4MMlE, once you have addressed the comments and added the tests, feel free to re-request my review and I will look at the code again. Thanks a lot in the meantime!
mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Collaborator
|
I just remembered one more comment I wanted to make that I forgot: You talk about how you no longer have to explicitly filter for control qubits due to the new dialect structure. Imagine the following pseudo-code: Here, the first so in theory, this can definitely be merged. Now, I don't know if this is a flaw of the pass (maybe this situation should be checked explicitly), a flaw of the dialect implementation (maybe My personal gut feeling is that it would be a nice helper method to implement for the |
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
aaa7096 to
94ea576
Compare
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
f0989ad to
045857a
Compare
DRovara
requested changes
Feb 2, 2026
Collaborator
DRovara
left a comment
DRovara
left a comment
There was a problem hiding this comment.
Thanks a lot @J4MMlE for the work. It looks super good already (for the record, I still haven't looked at the CMake configuration, I'll leave that to someone else).
The tests also look really really clean, I like that a lot.
I did have a minor concern regarding the numerical correctness, maybe you could check that out real quick? Either I am wrong or something is not quite working correctly.
Once my comments are addressed, feel free to remove the "Draft" status from this PR and request a review from Lukas.
mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/test_qco_quaternion_merge.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
042de31 to
290b626
Compare
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a quaternion-based MergeRotationGates MLIR pass, a Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant CLI as CLI (mqt-cc)
end
rect rgba(200,255,200,0.5)
participant Pipeline as CompilerPipeline
participant PM as PassManager
participant QCO as MLIRQCOTransforms
end
CLI->>Pipeline: set QuantumCompilerConfig.mergeSingleQubitRotationGates
Pipeline->>PM: build Stage 5 passes
alt mergeSingleQubitRotationGates == true
Pipeline->>PM: add MergeRotationGates pass
end
PM->>QCO: execute MergeRotationGates (pattern rewrites)
QCO-->>PM: success / failure
PM-->>Pipeline: continue remaining stages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@mlir/lib/Dialect/QCO/Transforms/CMakeLists.txt`:
- Line 11: The global add_compile_options(-fexceptions) affects all targets;
instead remove that line and add a scoped compile option on the
MLIRQCOTransforms target by calling target_compile_options(MLIRQCOTransforms
PRIVATE -fexceptions) after the MLIRQCOTransforms target is created (e.g., after
the add_library/add_mlir_dialect or similar target definition) so only
MLIRQCOTransforms gets -fexceptions.
- Line 13: Remove the stray debug output in the CMakeLists by deleting or
guarding the message(STATUS "MLIR_DIALECT_LIBS contains: ${dialect_libs}") line;
either remove that message entirely from
mlir/lib/Dialect/QCO/Transforms/CMakeLists.txt or wrap it behind a project-level
debug option (e.g., only call message when a DEBUG_CMAKE or similar variable is
enabled) so normal builds no longer emit the dialect_libs status line.
In `@mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp`:
- Around line 363-370: Fix the comment typo on the line above the SelectOp
creations: change "weather" to "whether" in the sentence "// choose correct
alpha and gamma weather safe or not" so it reads "// choose correct alpha and
gamma whether safe or not" (locate the comment immediately preceding the
creation of alpha and gamma via rewriter.create<mlir::arith::SelectOp>).
In `@mlir/unittests/Dialect/QCO/Transforms/CMakeLists.txt`:
- Around line 11-20: Remove or replace the TODO in the target_link_libraries
block for mqt-core-mlir-dialect-qco-transforms-test: either delete the "# TODO
figure out correct dependencies" line or update it to a concrete status/note
(e.g., "Verified dependencies for CI" or list missing deps) so it no longer
suggests unfinished work; verify the linked targets (GTest::gtest_main,
MLIRQCOProgramBuilder, MLIRQCOTransforms, MLIRIR, MLIRPass, MLIRSupport,
LLVMSupport) are intentionally present before committing the change.
In `@mlir/unittests/Dialect/QCO/Transforms/test_qco_quaternion_merge.cpp`:
- Around line 543-577: Move the SCF dialect load into the test fixture SetUp:
add context.loadDialect<scf::SCFDialect>(); inside the
QCOQuaternionMergeTest::SetUp() implementation so all tests consistently load
SCFDialect, and remove the duplicate context.loadDialect<scf::SCFDialect>();
call from the multipleUseInIf test (which currently calls it inside that test
body).
- Around line 136-153: The buildRotations function reads gate.angles[0..2] for
UOp without checking length, risking OOB; update buildRotations to validate the
RotationGate angles vector before calling builder.u by asserting or returning an
error when gate.opName == UOp::getOperationName() and gate.angles.size() < 3 (or
providing safe defaults), e.g. add a size check for gate.angles >= 3 and emit a
clear assertion/log message that includes the offending gate so the caller can
diagnose; reference buildRotations, RotationGate, UOp, gate.angles, and
builder.u when locating the change.
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Outdated
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
290b626 to
6c5018e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp`:
- Around line 136-144: Add a trailing llvm_unreachable() immediately after the
switch that handles RotationAxis to guard against future enum extensions and
silence non-exhaustive-switch warnings; locate the switch on axis (the one
returning quaternion literals for RotationAxis::X/Y/Z in
QuaternionMergeRotationGates.cpp) and append llvm_unreachable("Unhandled
RotationAxis") (and include the proper header <llvm/Support/ErrorHandling.h> if
not already included).
- Around line 309-316: The computed acos input bTemp3 (constructed from ww, zz,
bTemp1, bTemp2) can drift outside [-1,1]; clamp it to the valid domain before
calling mlir::math::AcosOp to avoid NaN. Replace the direct use of bTemp3 in the
AcosOp with a clamped value produced via mlir::arith::MinFOp and
mlir::arith::MaxFOp (or a combination) to bound it to [-1.0, 1.0] using the
existing constants (one, negOne or create negOne if needed); then pass that
clamped value to the creation of beta (rewriter.create<mlir::math::AcosOp>(loc,
clampedValue)). Ensure the new ops reference the same loc and use rewriter so
the IR stays consistent.
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
2 times, most recently
from
4cb71da to
c875d00
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mlir/lib/Dialect/QCO/Transforms/QuaternionMergeRotationGates.cpp`:
- Around line 374-375: The alpha/gamma extraction in
QuaternionMergeRotationGates.cpp can produce angles outside [-PI, PI]; implement
normalization for the variables alpha and gamma (e.g., in the code path that
computes these values inside the QuaternionMergeRotationGates transform/pass) by
mapping them into the principal range [-M_PI, M_PI] using a modular reduction
(fmod/remainder) and ±2*PI adjustment so mathematically equivalent angles are
canonical; update any uses of alpha/gamma in emit/replace logic within the
QuaternionMergeRotationGates routine to use the normalized values.
In `@mlir/unittests/Dialect/QCO/Transforms/test_qco_quaternion_merge.cpp`:
- Around line 702-715: Update the test comment in TEST_F(QCOQuaternionMergeTest,
numericalRotationIdentity) to match the assertion in the test
(expectUGateParams(0, TAU, 0)) rather than claiming U(0, 0, 0) or U(PI, -PI, 0);
reference the actual expected output U(0, TAU, 0) (with TAU = 2π) in the comment
or alternatively change the assertion to expect U(0, 0, 0) if that is the
intended canonical form—locate the test by the name numericalRotationIdentity
and the call expectUGateParams(0, TAU, 0) to make the consistent update.
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
2 times, most recently
from
8ff7ccc to
f3cbe5b
Compare
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
b73951e to
51fbbfa
Compare
Author
|
@burgholzer its ready for review again! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)
591-605:⚠️ Potential issue | 🟠 MajorReintroduce an explicit insertion-point guard before building the merged chain.
All
arith::/math::ops for the merged quaternion are created at the current rewrite insertion point, which is still the chain head here. If a later gate angle is produced by anarithop between chain elements, the new ops will be inserted before that definition and violate SSA dominance. Please move the insertion point tochain.back().getOperation()(or just before the first post-chain consumer) while materializingconstants,qAccum, andnewOp.🐛 Proposed fix
auto chain = collectChain(op); if (chain.size() < 2) { return failure(); } + OpBuilder::InsertionGuard guard(rewriter); + rewriter.setInsertionPoint(chain.back().getOperation()); + auto loc = op->getLoc(); auto constants = createConstants(loc, rewriter); // Initialize quaternion accumulator from the first operation auto qAccum = quaternionFromRotation(chain.front(), constants, rewriter);Run the following script to confirm the missing insertion-point adjustment and the lack of a regression test with late-defined scalar angles:
#!/bin/bash set -euo pipefail echo "=== matchAndRewrite body ===" fd QuaternionMergeRotationGates.cpp --exec sed -n '580,610p' {} echo echo "=== quaternion-merge tests using arithmetic between mergeable gates ===" fd test_qco_quaternion_merge.cpp --exec rg -n 'arith\.(addf|subf|mulf|divf)' {}Expected result:
matchAndRewriteshows nosetInsertionPoint*call before buildingconstants/qAccum/newOp, and the test search returns no coverage for a late-defined angle value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 591 - 605, The merged-quaternion ops are currently created at the rewrite's existing insertion point (head of the chain), risking SSA dominance if a later gate angle is produced by an arith op; before calling createConstants, quaternionFromRotation (for the first op), the loop that folds with hamiltonProduct, and before uOpFromQuaternion, set the rewriter insertion point to chain.back().getOperation() (or the operation immediately before the first post-chain consumer) so all arith::/math:: ops for the merged quaternion are emitted after the chain elements; locate these calls (createConstants, quaternionFromRotation, hamiltonProduct, uOpFromQuaternion) and wrap them with a temporary insertion-point guard (setInsertionPoint / setInsertionPointAfter or equivalent) and restore the original insertion point after newOp is created.
🤖 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/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 555-568: Remove the stale pairwise helper by deleting the
createOpQuaternionMergedAngle function definition (and any forward declaration)
and any references to it so there is only the chain-based merge path
(matchAndRewrite) left; search for the symbol createOpQuaternionMergedAngle and
remove its declaration/definition from QuaternionMergeRotationGates.cpp and any
header, ensure no callers remain, remove any now-unused includes or using
directives, and run the build/static analysis to confirm the unused-function
warning is resolved.
- Around line 367-376: collectChain currently dereferences
current->getUsers().begin() before verifying there is a user, which causes UB
for a dead tail; update collectChain to first check that current->getUsers() is
non-empty (and ideally that it has exactly one user if that's required) before
dereferencing, then fetch the user, test
areQuaternionMergeable(*current.getOperation(), *userOp) and only then push/cast
and advance current; modify the loop around current, userOp and the
areQuaternionMergeable call so dereferencing never happens when getUsers() is
empty.
In `@mlir/tools/mqt-cc/mqt-cc.cpp`:
- Around line 74-77: The cl::opt option mergeSingleQubitRotationGates is
declared const but must follow the file convention of mutable command-line
options; remove the const and declare it as static cl::opt<bool>
mergeSingleQubitRotationGates(...) so ParseCommandLineOptions can update it and
the declaration matches other options in this file.
In `@mlir/unittests/Compiler/test_compiler_pipeline.cpp`:
- Around line 195-199: The test comment is inaccurate: it claims the test
compares outputs “with and without the pass enabled” but the code actually
compares afterQCOCanon and afterOptimization from a single run; update the
comment to accurately describe the current comparison logic (that the test
compares the optimization output stages after running the pass, specifically
comparing the afterQCOCanon and afterOptimization results) and remove or reword
the “with and without the pass enabled” phrasing so it matches the behavior
around afterQCOCanon and afterOptimization in test_compiler_pipeline.cpp.
In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp`:
- Around line 503-517: The test currently uses two RX gates (builder.rx) which
don't prove cross-qubit safety because same-axis RX+RX are intentionally not
merged; change the second gate to a different mergeable type (e.g.,
builder.ry(1.0, qubit2) or a UOp) so the pass must respect qubit identity, then
update the assertions: keep invoking runMergePass(module.get()) and assert that
both separate rotations remain (e.g., EXPECT_EQ(countOps<RXOp>(), 1) and
EXPECT_EQ(countOps<RYOp>(), 1)) and/or assert that no UOp was produced
(EXPECT_EQ(countOps<UOp>(), 0)); target symbols:
QCOQuaternionMergeTest::dontMergeGatesFromDifferentQubits, builder.rx,
builder.ry (or UOp), runMergePass, countOps<RXOp>(), countOps<RYOp>(),
countOps<UOp>().
---
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 591-605: The merged-quaternion ops are currently created at the
rewrite's existing insertion point (head of the chain), risking SSA dominance if
a later gate angle is produced by an arith op; before calling createConstants,
quaternionFromRotation (for the first op), the loop that folds with
hamiltonProduct, and before uOpFromQuaternion, set the rewriter insertion point
to chain.back().getOperation() (or the operation immediately before the first
post-chain consumer) so all arith::/math:: ops for the merged quaternion are
emitted after the chain elements; locate these calls (createConstants,
quaternionFromRotation, hamiltonProduct, uOpFromQuaternion) and wrap them with a
temporary insertion-point guard (setInsertionPoint / setInsertionPointAfter or
equivalent) and restore the original insertion point after newOp is created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac9eb884-6cfb-41bb-bec4-2be8b1a0698c
📒 Files selected for processing (11)
CHANGELOG.mdmlir/include/mlir/Compiler/CompilerPipeline.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Compiler/CMakeLists.txtmlir/lib/Compiler/CompilerPipeline.cppmlir/lib/Dialect/QCO/Transforms/CMakeLists.txtmlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Outdated
Show resolved
Hide resolved
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)
555-568: 🧹 Nitpick | 🔵 TrivialRemove the stale pairwise merge helper.
createOpQuaternionMergedAngleis no longer referenced after the chain-based rewrite. Keeping a second merge path here makes future fixes easy to miss, and Cppcheck is already flagging it as unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 555 - 568, Remove the now-unused pairwise merge helper by deleting the createOpQuaternionMergedAngle function definition (and any corresponding forward declaration) and any code that references it; locate the function signature using the symbols UnitaryOpInterface and PatternRewriter and the body that calls quaternionFromRotation, hamiltonProduct, and uOpFromQuaternion, then remove that entire helper to avoid a duplicate merge path and silence the Cppcheck unused warning.
🤖 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/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 597-604: qAccum may drift from unit length after repeated
hamiltonProduct calls (from quaternionFromRotation and hamiltonProduct) and
uOpFromQuaternion assumes a unit quaternion; add a normalize step before calling
uOpFromQuaternion: implement a helper normalizeQuaternion(Quaternion, Location,
const Constants&, PatternRewriter&) that computes sqrt(w*w+x*x+y*y+z*z), divides
each component by the norm (using math::SqrtOp and arithmetic ops), and replace
the direct use of qAccum with the normalized quaternion when invoking
uOpFromQuaternion.
---
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 555-568: Remove the now-unused pairwise merge helper by deleting
the createOpQuaternionMergedAngle function definition (and any corresponding
forward declaration) and any code that references it; locate the function
signature using the symbols UnitaryOpInterface and PatternRewriter and the body
that calls quaternionFromRotation, hamiltonProduct, and uOpFromQuaternion, then
remove that entire helper to avoid a duplicate merge path and silence the
Cppcheck unused warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce48f6a1-6e52-4dad-a4aa-afd4a413b5f0
📒 Files selected for processing (6)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
51fbbfa to
b2a7d4b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp (1)
132-141:⚠️ Potential issue | 🟡 MinorCompare
Uangles modulo periodicity to prevent false-negative tests.
expectUGateParamscurrently does direct numeric comparison, but these parameters are periodic; the identity test even documents two equivalent forms (phi = 0or2π) while asserting only one. This can fail on equivalent outputs.♻️ Proposed fix
+ static double circularDiff(double a, double b, double period) { + const double diff = std::fmod(std::abs(a - b), period); + return std::min(diff, period - diff); + } + void expectUGateParams(double expectedTheta, double expectedPhi, double expectedLambda, double tolerance = 1e-8) { auto params = getUGateParams(); ASSERT_TRUE(params.has_value()); auto [theta, phi, lambda] = *params; - EXPECT_NEAR(theta, expectedTheta, tolerance); - EXPECT_NEAR(phi, expectedPhi, tolerance); - EXPECT_NEAR(lambda, expectedLambda, tolerance); + EXPECT_NEAR(circularDiff(theta, expectedTheta, 4.0 * PI), 0.0, tolerance); + EXPECT_NEAR(circularDiff(phi, expectedPhi, TAU), 0.0, tolerance); + EXPECT_NEAR(circularDiff(lambda, expectedLambda, TAU), 0.0, tolerance); }Based on learnings: In MQT Core’s OpenQASM 2
Udefinition,thetais 4π periodic whilephiandlambdaare 2π periodic.Also applies to: 741-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp` around lines 132 - 141, The test's expectUGateParams does direct numeric comparisons causing false negatives for equivalent U gates; update expectUGateParams (and the other similar assertions around the second block referencing U parameters) to compare angles modulo periodicity: normalize theta differences modulo 4π and phi/lambda modulo 2π (e.g., compute minimal wrap-around difference between expected and actual angle and then use EXPECT_NEAR on that minimal difference with the given tolerance); locate getUGateParams usage and replace the three EXPECT_NEAR checks with wrapped-comparison logic so equivalent angles like 0 vs 2π (or theta differing by 4π) no longer fail.mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (3)
601-608:⚠️ Potential issue | 🟠 MajorRenormalize
qAccumbefore converting back to Euler angles.
uOpFromQuaternionassumes a unit quaternion, but this path now chains several Hamilton products without the old quaternion→Euler→quaternion reset. Floating-point drift inqAccumwill skewbeta, and near the gimbal-lock thresholds it can also flip thesafebranch. Normalize once before Line 608.➕ Suggested fix
for (auto chainOp : llvm::drop_begin(chain)) { auto qi = quaternionFromRotation(chainOp, constants, rewriter); qAccum = hamiltonProduct(qi, qAccum, loc, rewriter); } + qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter); // Convert merged quaternion back to UOp auto newOp = uOpFromQuaternion(qAccum, op, constants, rewriter);static Quaternion normalizeQuaternion(Quaternion q, Location loc, const Constants& constants, PatternRewriter& rewriter) { auto ww = arith::MulFOp::create(rewriter, loc, q.w, q.w); auto xx = arith::MulFOp::create(rewriter, loc, q.x, q.x); auto yy = arith::MulFOp::create(rewriter, loc, q.y, q.y); auto zz = arith::MulFOp::create(rewriter, loc, q.z, q.z); auto sum1 = arith::AddFOp::create(rewriter, loc, ww, xx); auto sum2 = arith::AddFOp::create(rewriter, loc, yy, zz); auto normSq = arith::AddFOp::create(rewriter, loc, sum1, sum2); auto norm = math::SqrtOp::create(rewriter, loc, normSq); auto invNorm = arith::DivFOp::create(rewriter, loc, constants.one, norm); return { .w = arith::MulFOp::create(rewriter, loc, q.w, invNorm), .x = arith::MulFOp::create(rewriter, loc, q.x, invNorm), .y = arith::MulFOp::create(rewriter, loc, q.y, invNorm), .z = arith::MulFOp::create(rewriter, loc, q.z, invNorm), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 601 - 608, Normalize qAccum before converting back to Euler angles: insert a call to a new helper (e.g., normalizeQuaternion) that takes the Quaternion qAccum, Location loc, Constants& constants, and PatternRewriter& rewriter, computes its magnitude via w^2+x^2+y^2+z^2, sqrt, and multiplies each component by 1/norm to produce a unit quaternion, then pass the normalized result into uOpFromQuaternion instead of raw qAccum; implement normalizeQuaternion returning a Quaternion and call it just before the uOpFromQuaternion(qAccum, op, constants, rewriter) invocation.
595-608:⚠️ Potential issue | 🔴 CriticalBuild the merged quaternion where every later parameter dominates.
Line 603 can consume angle SSA values from gates that appear after
chain.front(), but the current insertion point is still at the chain head. That emitsarith/mathops using non-dominating operands as soon as a later rotation angle is computed between the head and tail, which makes the rewritten IR invalid. Move the whole merge computation under an insertion guard to a point beforechain.back().🛠️ Suggested fix
auto loc = op->getLoc(); - auto constants = createConstants(loc, rewriter); + OpBuilder::InsertionGuard guard(rewriter); + rewriter.setInsertionPoint(chain.back().getOperation()); + auto constants = createConstants(loc, rewriter); // Initialize quaternion accumulator from the first operation auto qAccum = quaternionFromRotation(chain.front(), constants, rewriter);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 595 - 608, The merge loop computes and emits arithmetic using SSA values from later gates while the rewriter insertion point is still at the chain head; move the entire quaternion-merge computation (creation of constants, quaternionFromRotation calls, hamiltonProduct folding, and uOpFromQuaternion) under an insertion-point guard so all new ops are inserted at a location that dominates all consumed angle SSA values — specifically set the rewriter insertion point to just before chain.back() (or use a ScopedInsertionPoint/rewriter.setInsertionPoint before chain.back()) before calling createConstants, quaternionFromRotation, hamiltonProduct, and uOpFromQuaternion to ensure later parameters dominate.
547-572: 🧹 Nitpick | 🔵 TrivialRemove the stale pairwise merge helper.
createOpQuaternionMergedAngleno longer has callers after the chain-based rewrite landed, so it is now dead code and a second merge path that can drift frommatchAndRewrite. Please delete it instead of maintaining both implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 547 - 572, Delete the now-dead pairwise merge helper function createOpQuaternionMergedAngle and any forward declarations or declarations referring to it so only the chain-based merge remains; remove the entire static function body (and any comments immediately attached) from QuaternionMergeRotationGates.cpp, search for and eliminate any remaining references to createOpQuaternionMergedAngle (including tests or headers) so compilation still succeeds and matchAndRewrite remains the single merge implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 601-608: Normalize qAccum before converting back to Euler angles:
insert a call to a new helper (e.g., normalizeQuaternion) that takes the
Quaternion qAccum, Location loc, Constants& constants, and PatternRewriter&
rewriter, computes its magnitude via w^2+x^2+y^2+z^2, sqrt, and multiplies each
component by 1/norm to produce a unit quaternion, then pass the normalized
result into uOpFromQuaternion instead of raw qAccum; implement
normalizeQuaternion returning a Quaternion and call it just before the
uOpFromQuaternion(qAccum, op, constants, rewriter) invocation.
- Around line 595-608: The merge loop computes and emits arithmetic using SSA
values from later gates while the rewriter insertion point is still at the chain
head; move the entire quaternion-merge computation (creation of constants,
quaternionFromRotation calls, hamiltonProduct folding, and uOpFromQuaternion)
under an insertion-point guard so all new ops are inserted at a location that
dominates all consumed angle SSA values — specifically set the rewriter
insertion point to just before chain.back() (or use a
ScopedInsertionPoint/rewriter.setInsertionPoint before chain.back()) before
calling createConstants, quaternionFromRotation, hamiltonProduct, and
uOpFromQuaternion to ensure later parameters dominate.
- Around line 547-572: Delete the now-dead pairwise merge helper function
createOpQuaternionMergedAngle and any forward declarations or declarations
referring to it so only the chain-based merge remains; remove the entire static
function body (and any comments immediately attached) from
QuaternionMergeRotationGates.cpp, search for and eliminate any remaining
references to createOpQuaternionMergedAngle (including tests or headers) so
compilation still succeeds and matchAndRewrite remains the single merge
implementation.
In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp`:
- Around line 132-141: The test's expectUGateParams does direct numeric
comparisons causing false negatives for equivalent U gates; update
expectUGateParams (and the other similar assertions around the second block
referencing U parameters) to compare angles modulo periodicity: normalize theta
differences modulo 4π and phi/lambda modulo 2π (e.g., compute minimal
wrap-around difference between expected and actual angle and then use
EXPECT_NEAR on that minimal difference with the given tolerance); locate
getUGateParams usage and replace the three EXPECT_NEAR checks with
wrapped-comparison logic so equivalent angles like 0 vs 2π (or theta differing
by 4π) no longer fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5ced74e-9e8e-46f7-bb09-8a1038b2d2fe
📒 Files selected for processing (4)
mlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (1)
574-581: 🧹 Nitpick | 🔵 TrivialConsider renormalizing the accumulated quaternion before Euler extraction.
After chaining multiple Hamilton products (lines 575-578), floating-point drift can move
|qAccum|slightly off 1.0. While theacosinput clamping at lines 481-484 partially mitigates this forbeta, explicit renormalization would provide additional robustness for longer chains:// After the Hamilton product loop: // qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter);This is a minor robustness enhancement rather than a correctness bug—for typical chain lengths (2-5 gates), the accumulated error is negligible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 574 - 581, After accumulating quaternions with quaternionFromRotation and hamiltonProduct into qAccum, normalize qAccum before converting back to a UOp to avoid FP drift: insert a call to a quaternion normalization helper (e.g., normalizeQuaternion that computes the quaternion norm and divides components using constants/rewriter and loc) immediately after the Hamilton product loop and before uOpFromQuaternion(qAccum, op, constants, rewriter); if no normalizeQuaternion exists, add one that uses the existing constants/rewriter pattern to compute 1.0 / sqrt(sumsq) and multiply qAccum components by that scalar.
🤖 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/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 374-380: The while loop body with the single-statement sequence
that gets the user and checks mergeability (using current, userOp,
areQuaternionMergeable, chain and UnitaryOpInterface) lacks braces; add braces
around the entire body of the while loop so the block that computes userOp,
tests areQuaternionMergeable(*current.getOperation(), *userOp), pushes to chain,
and updates current is enclosed in { ... } for consistent style and to avoid
future accidental errors.
- Around line 608-627: Move the MergeRotationGates pass struct into the existing
anonymous namespace to give it internal linkage: relocate the definition of
struct MergeRotationGates (which inherits
impl::MergeRotationGatesBase<MergeRotationGates>) inside the file's anonymous
namespace and keep its override of runOnOperation unchanged; ensure
populateMergeRotationGatesPatterns remains accessible to the pass (either by
leaving that function in the anonymous namespace or converting it to a method)
so the call inside runOnOperation continues to compile.
- Around line 15-25: The build may rely on a transitive include for
llvm::drop_begin used in QuaternionMergeRotationGates.cpp; add an explicit
include of the header that defines it by importing <llvm/ADT/STLExtras.h> at the
top of the file so references to llvm::drop_begin(chain) are resolved directly
(look for usages of llvm::drop_begin and the file
QuaternionMergeRotationGates.cpp to place the new include alongside the other
llvm includes).
---
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 574-581: After accumulating quaternions with
quaternionFromRotation and hamiltonProduct into qAccum, normalize qAccum before
converting back to a UOp to avoid FP drift: insert a call to a quaternion
normalization helper (e.g., normalizeQuaternion that computes the quaternion
norm and divides components using constants/rewriter and loc) immediately after
the Hamilton product loop and before uOpFromQuaternion(qAccum, op, constants,
rewriter); if no normalizeQuaternion exists, add one that uses the existing
constants/rewriter pattern to compute 1.0 / sqrt(sumsq) and multiply qAccum
components by that scalar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73653c15-2a0d-4626-9811-68c8d547c584
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
Member
Could you please first address the linter warnings as well as the open CodeRabbit comments? |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (2)
571-580:⚠️ Potential issue | 🟠 MajorRenormalize
qAccumbefore converting it back to Euler angles.
uOpFromQuaternionassumes a unit quaternion, but Lines 574-577 now fold an arbitrary-length Hamilton-product chain without reprojecting onto the unit sphere. That drift skewsbetaand can flip the gimbal-lock branch near0/PI. A smallnormalizeQuaternion(...)helper that divides bysqrt(w*w + x*x + y*y + z*z)is enough here.♻️ Proposed fix
for (auto chainOp : llvm::drop_begin(chain)) { auto qi = quaternionFromRotation(chainOp, constants, rewriter); qAccum = hamiltonProduct(qi, qAccum, loc, rewriter); } + qAccum = normalizeQuaternion(qAccum, loc, constants, rewriter); // Convert merged quaternion back to UOp auto newOp = uOpFromQuaternion(qAccum, op, constants, rewriter);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 571 - 580, After folding the chain into qAccum via quaternionFromRotation and hamiltonProduct, renormalize qAccum before converting back to Euler with uOpFromQuaternion: add or call a normalizeQuaternion(qAccum) helper that divides each component by sqrt(w*w + x*x + y*y + z*z) (and defensively handle zero/nan norm), then pass the normalized quaternion to uOpFromQuaternion to avoid drift and gimbal-lock flips.
567-580:⚠️ Potential issue | 🔴 CriticalMove the rewrite insertion point to the chain tail before building the quaternion IR.
Line 568 still creates the helper arith/math ops at the pattern’s current insertion point. If a later gate in the chain gets its angle from an SSA value defined between the head and tail, those new ops will reference that value before it dominates, and the rewrite emits invalid IR. Use an
InsertionGuardand set the insertion point tochain.back().getOperation()beforecreateConstants,quaternionFromRotation, anduOpFromQuaternion, then add a regression where the second angle is produced by anarith.addfbetween two mergeable gates.♻️ Proposed fix
auto loc = op->getLoc(); + OpBuilder::InsertionGuard guard(rewriter); + rewriter.setInsertionPoint(chain.back().getOperation()); auto constants = createConstants(loc, rewriter);#!/bin/bash echo "=== matchAndRewrite insertion point ===" sed -n '556,586p' mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp echo echo "=== current test coverage for SSA-computed angle values ===" rg -n "arith\\.(addf|subf|mulf|divf)|builder\\.(rx|ry|rz|p|r|u2|u)\\(" mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 567 - 580, The rewrite currently creates helper arith/math ops at the pattern's original insertion point, which can break SSA dominance; wrap the rewrite with an mlir::OpBuilder::InsertionGuard and call rewriter.setInsertionPointAfter(chain.back().getOperation()) (or setInsertionPoint(chain.back().getOperation())) before calling createConstants, quaternionFromRotation, and uOpFromQuaternion so all helper ops are emitted at the chain tail; update the code locations around qAccum/quaternionFromRotation/hamiltonProduct/uOpFromQuaternion to use the guarded inserter and add a unit test where the second gate's angle is computed via an arith.addf between two mergeable gates to verify correct dominance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 571-580: After folding the chain into qAccum via
quaternionFromRotation and hamiltonProduct, renormalize qAccum before converting
back to Euler with uOpFromQuaternion: add or call a normalizeQuaternion(qAccum)
helper that divides each component by sqrt(w*w + x*x + y*y + z*z) (and
defensively handle zero/nan norm), then pass the normalized quaternion to
uOpFromQuaternion to avoid drift and gimbal-lock flips.
- Around line 567-580: The rewrite currently creates helper arith/math ops at
the pattern's original insertion point, which can break SSA dominance; wrap the
rewrite with an mlir::OpBuilder::InsertionGuard and call
rewriter.setInsertionPointAfter(chain.back().getOperation()) (or
setInsertionPoint(chain.back().getOperation())) before calling createConstants,
quaternionFromRotation, and uOpFromQuaternion so all helper ops are emitted at
the chain tail; update the code locations around
qAccum/quaternionFromRotation/hamiltonProduct/uOpFromQuaternion to use the
guarded inserter and add a unit test where the second gate's angle is computed
via an arith.addf between two mergeable gates to verify correct dominance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bacb27c-224d-4f9d-9129-a39f77fba6e8
📒 Files selected for processing (2)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
J4MMlE
commented
Mar 22, 2026
mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
Outdated
Show resolved
Hide resolved
J4MMlE
commented
Mar 22, 2026
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 205-225: Update the docblocks for quaternionFromZYZ and the
UOp/ZYZ helper functions to explicitly state that the quaternion computed
represents the ZYZ decomposition only up to a global phase relative to the
OpenQASM2 U(theta,phi,lambda) matrix; i.e., call out that the quaternionization
is exact modulo a global phase and therefore not phase-sensitive, and add the
same caveat to the other helper docblocks mentioned (the other ZYZ/UOp helpers).
Ensure the wording names the functions (quaternionFromZYZ and the UOp/ZYZ
helpers) so future users know these helpers are not suitable for phase-sensitive
contexts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6034b88-8205-45e5-8b89-2146cc3d279d
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp
Show resolved
Hide resolved
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
5fb5edc to
e606175
Compare
J4MMlE
commented
Mar 22, 2026
| }; | ||
|
|
||
| /** | ||
| * @brief Checks if an operation is a mergeable rotation gate. |
Author
There was a problem hiding this comment.
This function already is pretty self-explanatory so I don't know if such a long docstring is justified. Might delete...
J4MMlE
commented
Mar 22, 2026
|
|
||
| protected: | ||
| void runOnOperation() override { | ||
| // Get the current operation being operated on. |
Author
There was a problem hiding this comment.
Should default MLIR boilerplate like this have comments? Delete?
J4MMlE
force-pushed
the
quaternion-rotation-merging
branch
from
e606175 to
212605a
Compare
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp (2)
193-201:⚠️ Potential issue | 🟡 MinorRestore the trailing
llvm_unreachable()increateAxisQuaternion.The switch is exhaustive for today's enum, but this function still has a no-return path. That can reintroduce
-Wreturn-typenoise on some compilers and becomes UB ifRotationAxisever grows.♻️ Suggested fix
case RotationAxis::Z: return {.w = cos, .x = constants.zero, .y = constants.zero, .z = sin}; } + llvm_unreachable("Unhandled RotationAxis"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp` around lines 193 - 201, The switch in createAxisQuaternion returns for all current RotationAxis values but leaves a theoretical no-return path; restore a final llvm_unreachable() (or equivalent assert) after the switch in createAxisQuaternion to silence -Wreturn-type and prevent UB if RotationAxis is extended in the future, ensuring the function always has a terminal unreachable statement for the default/unhandled case.
85-94: 🧹 Nitpick | 🔵 TrivialUse MLIR RTTI instead of
getName()for the mergeability check.This predicate is defining chain boundaries, so coupling it to textual op names makes it more brittle than the
isa/TypeSwitchstyle you already use everywhere else in this file.mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp (1)
125-137:⚠️ Potential issue | 🟡 MinorCompare
Uparameters modulo their natural periods in the helper.
expectUGateParamscurrently treats equivalent outputs like0vs2πorπvs-πas different, so a mathematically correct rewrite can fail these tests after harmless canonicalization changes.♻️ Suggested fix
+ static double periodicDiff(double actual, double expected, double period) { + return std::abs(std::remainder(actual - expected, period)); + } + void expectUGateParams(double expectedTheta, double expectedPhi, double expectedLambda, double tolerance = 1e-8) { auto params = getUGateParams(); ASSERT_TRUE(params.has_value()); auto [theta, phi, lambda] = *params; - EXPECT_NEAR(theta, expectedTheta, tolerance); - EXPECT_NEAR(phi, expectedPhi, tolerance); - EXPECT_NEAR(lambda, expectedLambda, tolerance); + EXPECT_NEAR(periodicDiff(theta, expectedTheta, 4.0 * PI), 0.0, tolerance); + EXPECT_NEAR(periodicDiff(phi, expectedPhi, 2.0 * PI), 0.0, tolerance); + EXPECT_NEAR(periodicDiff(lambda, expectedLambda, 2.0 * PI), 0.0, + tolerance); }Based on learnings, in MQT Core the
Ugate uses the OpenQASM 2 definition, so θ is 4π-periodic while φ and λ are 2π-periodic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp` around lines 125 - 137, The helper expectUGateParams should compare angles modulo their natural periods instead of raw values: retrieve params via getUGateParams(), unwrap the optional as done now, then normalize theta to the canonical range modulo 4π and normalize phi and lambda modulo 2π (e.g., map to [-period/2, period/2] or [0,period) consistently) before calling EXPECT_NEAR; update expectUGateParams to perform these modular reductions so equivalent angles like 0 vs 2π or π vs -π are treated as equal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cpp`:
- Around line 193-201: The switch in createAxisQuaternion returns for all
current RotationAxis values but leaves a theoretical no-return path; restore a
final llvm_unreachable() (or equivalent assert) after the switch in
createAxisQuaternion to silence -Wreturn-type and prevent UB if RotationAxis is
extended in the future, ensuring the function always has a terminal unreachable
statement for the default/unhandled case.
In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp`:
- Around line 125-137: The helper expectUGateParams should compare angles modulo
their natural periods instead of raw values: retrieve params via
getUGateParams(), unwrap the optional as done now, then normalize theta to the
canonical range modulo 4π and normalize phi and lambda modulo 2π (e.g., map to
[-period/2, period/2] or [0,period) consistently) before calling EXPECT_NEAR;
update expectUGateParams to perform these modular reductions so equivalent
angles like 0 vs 2π or π vs -π are treated as equal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03f31963-5f48-44c3-9b5d-066d5d834544
📒 Files selected for processing (2)
mlir/lib/Dialect/QCO/Transforms/Optimizations/QuaternionMergeRotationGates.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_quaternion_merge.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR extends the rotation merging pass in the QCO
MQTOptdialect to support quaternion-based gate fusion. This is the first step toward closing #1029.The existing rotation merge pass only merges consecutive rotation gates of the same type (e.g.,
rx + rxorry + ry) by adding their angles.This PR introduces quaternion-based merging, which can merge rotation gates of different types (currently only single qubit gates
rx,ry,rz,r,p,u2,u).Quaternions are widely used to represent rotations in three-dimensional space and naturally map to qubit gate rotations around the Bloch sphere. The implementation:
ugate. (This could also be done differently in the future, and directly decompose to the correct base gates by using the decomposition from ✨ Implement single-qubit gate decomposition pass #1182)Since this optimization may only be beneficial on certain quantum architectures, it is disabled by default. It can be invoked using:
Checklist:
- [ ] I have added migration instructions to the upgrade guide (if needed).