✨ Implement two-qubit gate decomposition pass #1206
Conversation
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
d7f49cc to
4bc6f15
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
c4f6399 to
ffb96f3
Compare
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
2e3a48f to
f5ae2b0
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
3 times, most recently
from
bbcec1e to
49d512f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new gate decomposition pass is added to the MLIR MQTOpt dialect. The implementation includes pass declaration, Weyl-based two-qubit decomposition pattern logic, utility helpers for MLIR/quantum operations, CMake integration with Eigen dependency, and comprehensive MLIR tests. Changes
Sequence DiagramsequenceDiagram
participant Pass as GateDecomposition Pass
participant Pattern as GateDecompositionPattern
participant Decomposer as TwoQubitBasisDecomposer
participant Rewriter as PatternRewriter
Pass->>Pass: runOnOperation()
Pass->>Pass: Create RewritePatternSet
Pass->>Pattern: populateGateDecompositionPatterns()
Pattern->>Pass: Register pattern
Pass->>Pass: Configure GreedyRewriteConfig
loop For each Operation (top-down)
Pass->>Pattern: matchAndRewrite(UnitaryInterface op)
alt Pattern Matches
Pattern->>Decomposer: create() + twoQubitDecompose()
Decomposer->>Decomposer: Weyl decomposition
Decomposer->>Decomposer: Generate gate sequence
Decomposer-->>Pattern: Return TwoQubitSeries
Pattern->>Pattern: applySeries() - replace + validate
Pattern->>Rewriter: Emit new operations
Rewriter-->>Pass: Update IR
else No Match
Pattern-->>Pass: Skip operation
end
end
alt Success
Pass->>Pass: Pass succeeded
else Failure
Pass->>Pass: Signal pass failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(1 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h(1 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.td(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/CMakeLists.txt(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/GateDecomposition.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Helpers.h(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.tdmlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/GateDecomposition.cppmlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.hmlir/lib/Dialect/MQTOpt/Transforms/Helpers.hmlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/Helpers.h
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlirmlir/lib/Dialect/MQTOpt/Transforms/Helpers.hmlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
CMakeLists.txt
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
[warning] 1689-1689: mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp:1689:30 [misc-include-cleaner]
no header providing "std::numeric_limits" is directly included
[warning] 929-929: mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp:929:14 [misc-include-cleaner]
no header providing "Eigen::Vector" is directly included
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐉 Test / 🍎 macos-15-intel with LLVM@21
🔇 Additional comments (3)
mlir/lib/Dialect/MQTOpt/Transforms/CMakeLists.txt (1)
10-10: Confirm thatEigen3::Eigenis the correct target from your Eigen FetchContent setupLinking against
Eigen3::Eigenhere is fine as long as the FetchContent configuration really defines that target on all supported toolchains; otherwise this will fail to configure or link in some environments. Please double‑check the Eigen CMake export to ensure the target name and namespace match what you use here (or add an alias/guard if necessary).mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h (1)
38-38: New pattern population hook looks consistent with existing transformsThe
populateGateDecompositionPatterns(RewritePatternSet&)declaration fits cleanly alongside the other populate* helpers and matches the usage inGateDecomposition.cpp. Just make sure the implementation inGateDecompositionPattern.cppis actually compiled intoMLIRMQTOptTransformsso the call site links correctly.mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp (1)
460-540: Creation ofGPhaseOpignores potential control qubits.When applying a series, the pattern adds a global phase gate via:
if (sequence.hasGlobalPhase()) { createOneParameterGate<GPhaseOp>(rewriter, location, sequence.globalPhase, {}); }This always creates a GPhase with empty
inQubits, even if the original series might have had controlled global phases. Given the dialect definition allowsGPhaseOpto be controlled by qubits, this may drop control wiring when decomposing controlled global-phase structures. (mqt.readthedocs.io)Please double‑check whether controlled GPhaseOps can appear in practice. If they can, you likely want to thread the relevant control qubits into the newly created
GPhaseOp(and its types) instead of using an emptyValueRange.⛔ Skipped due to learnings
Learnt from: DRovara Repo: munich-quantum-toolkit/core PR: 1108 File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288 Timestamp: 2025-10-09T13:20:11.483Z Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.Learnt from: DRovara Repo: munich-quantum-toolkit/core PR: 1108 File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180 Timestamp: 2025-10-09T13:13:51.224Z Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.Learnt from: burgholzer Repo: munich-quantum-toolkit/core PR: 1283 File: src/qir/runtime/QIR.cpp:196-201 Timestamp: 2025-11-01T15:57:31.153Z Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
| bool appendTwoQubitGate(UnitaryInterface nextGate) { | ||
| auto opInQubits = nextGate.getAllInQubits(); | ||
| auto&& firstOperand = opInQubits[0]; | ||
| auto&& secondOperand = opInQubits[1]; | ||
| auto* firstQubitIt = llvm::find(outQubits, firstOperand); | ||
| auto* secondQubitIt = llvm::find(outQubits, secondOperand); | ||
| if (firstQubitIt == outQubits.end() || secondQubitIt == outQubits.end()) { | ||
| // another qubit is involved, series is finished (except there only | ||
| // has been one qubit so far) | ||
| auto* it = llvm::find(outQubits, mlir::Value{}); | ||
| if (it == outQubits.end()) { | ||
| return false; | ||
| } | ||
| // iterator in the operation input of "old" qubit that already has | ||
| // previous single-qubit gates in this series | ||
| auto it2 = llvm::find(opInQubits, firstQubitIt != outQubits.end() | ||
| ? *firstQubitIt | ||
| : *secondQubitIt); | ||
| // new qubit ID based on position in outQubits | ||
| const QubitId newInQubitId = std::distance(outQubits.begin(), it); | ||
| // position in operation input; since there are only two qubits, it must | ||
| // be the "not old" one | ||
| const QubitId newOpInQubitId = | ||
| 1 - std::distance(opInQubits.begin(), it2); | ||
|
|
||
| // update inQubit and update dangling iterator, then proceed as usual | ||
| inQubits[newInQubitId] = opInQubits[newOpInQubitId]; | ||
| firstQubitIt = (firstQubitIt != outQubits.end()) ? firstQubitIt : it; | ||
| secondQubitIt = (secondQubitIt != outQubits.end()) ? secondQubitIt : it; | ||
|
|
||
| // before proceeding as usual, see if backtracking on the "new" qubit is | ||
| // possible to collect other single-qubit operations | ||
| backtrackSingleQubitSeries(newInQubitId); | ||
| } | ||
| const QubitId firstQubitId = | ||
| std::distance(outQubits.begin(), firstQubitIt); | ||
| const QubitId secondQubitId = | ||
| std::distance(outQubits.begin(), secondQubitIt); | ||
| *firstQubitIt = nextGate->getResult(0); | ||
| *secondQubitIt = nextGate->getResult(1); | ||
|
|
||
| gates.push_back( | ||
| {.op = nextGate, .qubitIds = {firstQubitId, secondQubitId}}); | ||
| complexity += getComplexity(helpers::getQcType(nextGate), 2); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Iterator misuse: drop the * when capturing result of llvm::find.
In appendTwoQubitGate, llvm::find returns an iterator over outQubits (for std::array, this is already a pointer-like iterator). Declaring the variables with auto* makes them “pointer to iterator”, which breaks comparisons against outQubits.end() and use in std::distance, and should fail to compile.
Fix by storing the iterator directly:
- auto* firstQubitIt = llvm::find(outQubits, firstOperand);
- auto* secondQubitIt = llvm::find(outQubits, secondOperand);
+ auto firstQubitIt = llvm::find(outQubits, firstOperand);
+ auto secondQubitIt = llvm::find(outQubits, secondOperand);The rest of the logic (firstQubitIt == outQubits.end(), std::distance(outQubits.begin(), firstQubitIt), etc.) then operates on iterators as intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool appendTwoQubitGate(UnitaryInterface nextGate) { | |
| auto opInQubits = nextGate.getAllInQubits(); | |
| auto&& firstOperand = opInQubits[0]; | |
| auto&& secondOperand = opInQubits[1]; | |
| auto* firstQubitIt = llvm::find(outQubits, firstOperand); | |
| auto* secondQubitIt = llvm::find(outQubits, secondOperand); | |
| if (firstQubitIt == outQubits.end() || secondQubitIt == outQubits.end()) { | |
| // another qubit is involved, series is finished (except there only | |
| // has been one qubit so far) | |
| auto* it = llvm::find(outQubits, mlir::Value{}); | |
| if (it == outQubits.end()) { | |
| return false; | |
| } | |
| // iterator in the operation input of "old" qubit that already has | |
| // previous single-qubit gates in this series | |
| auto it2 = llvm::find(opInQubits, firstQubitIt != outQubits.end() | |
| ? *firstQubitIt | |
| : *secondQubitIt); | |
| // new qubit ID based on position in outQubits | |
| const QubitId newInQubitId = std::distance(outQubits.begin(), it); | |
| // position in operation input; since there are only two qubits, it must | |
| // be the "not old" one | |
| const QubitId newOpInQubitId = | |
| 1 - std::distance(opInQubits.begin(), it2); | |
| // update inQubit and update dangling iterator, then proceed as usual | |
| inQubits[newInQubitId] = opInQubits[newOpInQubitId]; | |
| firstQubitIt = (firstQubitIt != outQubits.end()) ? firstQubitIt : it; | |
| secondQubitIt = (secondQubitIt != outQubits.end()) ? secondQubitIt : it; | |
| // before proceeding as usual, see if backtracking on the "new" qubit is | |
| // possible to collect other single-qubit operations | |
| backtrackSingleQubitSeries(newInQubitId); | |
| } | |
| const QubitId firstQubitId = | |
| std::distance(outQubits.begin(), firstQubitIt); | |
| const QubitId secondQubitId = | |
| std::distance(outQubits.begin(), secondQubitIt); | |
| *firstQubitIt = nextGate->getResult(0); | |
| *secondQubitIt = nextGate->getResult(1); | |
| gates.push_back( | |
| {.op = nextGate, .qubitIds = {firstQubitId, secondQubitId}}); | |
| complexity += getComplexity(helpers::getQcType(nextGate), 2); | |
| return true; | |
| } | |
| bool appendTwoQubitGate(UnitaryInterface nextGate) { | |
| auto opInQubits = nextGate.getAllInQubits(); | |
| auto&& firstOperand = opInQubits[0]; | |
| auto&& secondOperand = opInQubits[1]; | |
| auto firstQubitIt = llvm::find(outQubits, firstOperand); | |
| auto secondQubitIt = llvm::find(outQubits, secondOperand); | |
| if (firstQubitIt == outQubits.end() || secondQubitIt == outQubits.end()) { | |
| // another qubit is involved, series is finished (except there only | |
| // has been one qubit so far) | |
| auto* it = llvm::find(outQubits, mlir::Value{}); | |
| if (it == outQubits.end()) { | |
| return false; | |
| } | |
| // iterator in the operation input of "old" qubit that already has | |
| // previous single-qubit gates in this series | |
| auto it2 = llvm::find(opInQubits, firstQubitIt != outQubits.end() | |
| ? *firstQubitIt | |
| : *secondQubitIt); | |
| // new qubit ID based on position in outQubits | |
| const QubitId newInQubitId = std::distance(outQubits.begin(), it); | |
| // position in operation input; since there are only two qubits, it must | |
| // be the "not old" one | |
| const QubitId newOpInQubitId = | |
| 1 - std::distance(opInQubits.begin(), it2); | |
| // update inQubit and update dangling iterator, then proceed as usual | |
| inQubits[newInQubitId] = opInQubits[newOpInQubitId]; | |
| firstQubitIt = (firstQubitIt != outQubits.end()) ? firstQubitIt : it; | |
| secondQubitIt = (secondQubitIt != outQubits.end()) ? secondQubitIt : it; | |
| // before proceeding as usual, see if backtracking on the "new" qubit is | |
| // possible to collect other single-qubit operations | |
| backtrackSingleQubitSeries(newInQubitId); | |
| } | |
| const QubitId firstQubitId = | |
| std::distance(outQubits.begin(), firstQubitIt); | |
| const QubitId secondQubitId = | |
| std::distance(outQubits.begin(), secondQubitIt); | |
| *firstQubitIt = nextGate->getResult(0); | |
| *secondQubitIt = nextGate->getResult(1); | |
| gates.push_back( | |
| {.op = nextGate, .qubitIds = {firstQubitId, secondQubitId}}); | |
| complexity += getComplexity(helpers::getQcType(nextGate), 2); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp around lines
348-393, the iterators from llvm::find are stored as pointer-to-iterator (auto*)
which is incorrect; change firstQubitIt, secondQubitIt and the local it (and any
other variables receiving llvm::find results in this scope) to plain iterator
types (use auto without *) so comparisons to outQubits.end(), std::distance, and
dereferences operate on iterators as intended; ensure any subsequent assignments
that expect an iterator (e.g., firstQubitIt = it) still use matching iterator
types.
There was a problem hiding this comment.
While I agree with this issue (in case the container is exchanged), since std::array's iterator is defined as value_type*, dropping the pointer here will result in linter warnings.
There was a problem hiding this comment.
@taminob, that's a fair point about the linter warnings. You have a few options to balance correctness with tooling ergonomics:
-
Use explicit iterator type to satisfy both correctness and linters:
typename std::array<mlir::Value, 2>::iterator firstQubitIt = llvm::find(outQubits, firstOperand);
or more concisely:
std::array<mlir::Value, 2>::iterator firstQubitIt = llvm::find(outQubits, firstOperand); -
Configure the linter to accept
autofor pointer types when they're actually iterators (e.g., suppress the specific warning for this pattern). -
Keep
auto*for now if the code compiles and works correctly in your environment, accepting the theoretical portability risk ifstd::array's iterator implementation changes.
Since the code is compiling and working, and you're aware of the trade-off, I'll leave the decision to you. The explicit iterator type would be the most robust solution if you want to avoid both the theoretical issue and the linter warnings.
🧠 Learnings used
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
There was a problem hiding this comment.
The first suggestion using decltype instead of the explicit type is good, but will trigger yet another linter warning (clang-tidy modernize-use-auto). @flowerthrower do you have an opinion on this topic?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@burgholzer I removed the * from the type now because the windows CI won't build otherwise, do you have a preferred way of resolving this issue? Putting NOLINT(readability-qualified-auto) everywhere would work, but is also messy - I don't see a good solution here.
There was a problem hiding this comment.
Hm. I remember stumbling over this as well. I can't quite remover where, but it could have been as part of the dialect rewrite. I think that I added a NOLINT for that single occurrence.
Stupid thought, but have you tried auto&? do any of the systems complain about that?
if there are multiple instances of the rule violation in one place, you can always sandwich them between // NOLINTBEGIN(...) and // NOLINTEND(...) to hide the warning in a moderately clean way.
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
| module { | ||
| // CHECK-LABEL: func.func @testTwoBasisGateDecomposition | ||
| func.func @testTwoBasisGateDecomposition() { | ||
| // CHECK-DAG: %[[C0:.*]] = arith.constant -1.5707963267 | ||
| // CHECK-DAG: %[[C1:.*]] = arith.constant -3.141592653 | ||
| // CHECK-DAG: %[[C2:.*]] = arith.constant 1.2502369157 | ||
| // CHECK-DAG: %[[C3:.*]] = arith.constant 1.8299117708 | ||
| // CHECK-DAG: %[[C4:.*]] = arith.constant 2.8733113535 | ||
| // CHECK-DAG: %[[C5:.*]] = arith.constant 3.0097427712 | ||
| // CHECK-DAG: %[[C6:.*]] = arith.constant 0.2614370492 | ||
| // CHECK-DAG: %[[C7:.*]] = arith.constant -0.131849882 | ||
| // CHECK-DAG: %[[C8:.*]] = arith.constant 2.500000e+00 | ||
| // CHECK-DAG: %[[C9:.*]] = arith.constant -1.570796326 | ||
| // CHECK-DAG: %[[C10:.*]] = arith.constant 1.570796326 | ||
| // CHECK-DAG: %[[C11:.*]] = arith.constant 0.785398163 | ||
|
|
||
| // CHECK: %[[Q0_0:.*]] = mqtopt.allocQubit | ||
| // CHECK: %[[Q1_0:.*]] = mqtopt.allocQubit | ||
|
|
||
| // CHECK-NOT: mqtopt.gphase(%[[ANY:.*]]) | ||
| // CHECK: %[[Q0_1:.*]] = mqtopt.ry(%[[C11]]) %[[Q0_0]] | ||
| // CHECK: %[[Q0_2:.*]] = mqtopt.rz(%[[C10]]) %[[Q0_1]] | ||
| // CHECK: %[[Q1_1:.*]] = mqtopt.rx(%[[C9]]) %[[Q1_0]] | ||
| // CHECK: %[[Q1_2:.*]] = mqtopt.ry(%[[C8]]) %[[Q1_1]] | ||
| // CHECK: %[[Q0_3:.*]], %[[Q1_3:.*]] = mqtopt.x() %[[Q0_2]] ctrl %[[Q1_2]] | ||
| // CHECK: %[[Q0_4:.*]] = mqtopt.rz(%[[C7]]) %[[Q0_3]] | ||
| // CHECK: %[[Q0_5:.*]] = mqtopt.ry(%[[C6]]) %[[Q0_4]] | ||
| // CHECK: %[[Q0_6:.*]] = mqtopt.rz(%[[C5]]) %[[Q0_5]] | ||
| // CHECK: %[[Q1_4:.*]] = mqtopt.rz(%[[C10]]) %[[Q1_3]] | ||
| // CHECK: %[[Q1_5:.*]] = mqtopt.ry(%[[C10]]) %[[Q1_4]] | ||
| // CHECK: %[[Q0_7:.*]], %[[Q1_6:.*]] = mqtopt.x() %[[Q0_6]] ctrl %[[Q1_5]] | ||
| // CHECK: %[[Q0_8:.*]] = mqtopt.rz(%[[C4]]) %[[Q0_7]] | ||
| // CHECK: %[[Q0_9:.*]] = mqtopt.ry(%[[C3]]) %[[Q0_8]] | ||
| // CHECK: %[[Q0_10:.*]] = mqtopt.rz(%[[C2]]) %[[Q0_9]] | ||
| // CHECK: %[[Q1_7:.*]] = mqtopt.rz(%[[C1]]) %[[Q1_6]] | ||
| // CHECK: %[[Q1_8:.*]] = mqtopt.rx(%[[C0]]) %[[Q1_7]] | ||
|
|
||
| // ensure no other operations are inserted | ||
| // CHECK-NOT: mqtopt.[[ANY:.*]](%[[ANY:.*]]) | ||
|
|
||
| // CHECK: mqtopt.deallocQubit %[[Q0_10]] | ||
| // CHECK: mqtopt.deallocQubit %[[Q1_8]] | ||
|
|
||
| %cst0 = arith.constant 2.5 : f64 | ||
| %cst1 = arith.constant 1.2 : f64 | ||
| %cst2 = arith.constant 0.5 : f64 | ||
|
|
||
| %q0_0 = mqtopt.allocQubit | ||
| %q1_0 = mqtopt.allocQubit | ||
|
|
||
| %q0_x, %q1_x = mqtopt.i() %q0_0 ctrl %q1_0: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_1 = mqtopt.h() %q0_x: !mqtopt.Qubit | ||
| %q1_1, %q0_2 = mqtopt.x() %q1_x ctrl %q0_1: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_3, %q1_2 = mqtopt.rzz(%cst0) %q0_2, %q1_1: !mqtopt.Qubit, !mqtopt.Qubit | ||
| %q1_3 = mqtopt.ry(%cst1) %q1_2: !mqtopt.Qubit | ||
| %q0_4 = mqtopt.rx(%cst1) %q0_3: !mqtopt.Qubit | ||
| %q0_5, %q1_4 = mqtopt.x() %q0_4 ctrl %q1_3: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_6 = mqtopt.rz(%cst2) %q0_5: !mqtopt.Qubit | ||
| // make series longer to enforce decomposition | ||
| %q0_7, %q1_5 = mqtopt.i() %q0_6 ctrl %q1_4: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_8, %q1_6 = mqtopt.i() %q0_7 ctrl %q1_5: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
|
|
||
| mqtopt.deallocQubit %q0_8 | ||
| mqtopt.deallocQubit %q1_6 | ||
|
|
||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Be cautious with exact floating-point constants in decomposition checks
The complex two-/three-basis decomposition tests assert specific arith.constant f64 literals (e.g., 1.00543528660, 0.43624604946, etc.) for the synthesized single-qubit rotations. This gives very strong coverage of the numeric behavior, but it also makes the tests potentially brittle:
- Small differences in libm implementations, compiler optimization levels, or Eigen’s internal algorithms could produce slightly different doubles for the same mathematical angles.
- MLIR’s printing of
f64viaAPFloatis deterministic for a given bit pattern, but if the underlying computation changes even in the low bits, the decimal strings will change and FileCheck will fail.
If you expect to run this across multiple platforms/toolchains, consider relaxing the checks to focus on structure and gate types, for example:
- Only checking that certain
arith.constantops exist (without pinning exact values), or - Encoding angles in a way that is less sensitive to small numerical variation (e.g., fixed symbolic multiples of π where applicable).
Right now, a small numeric change in the decomposition algorithm will require updating a lot of golden numbers in this file.
Also applies to: 352-432
🏁 Script executed:
#!/bin/bash
# First, let's examine the test file structure
fd gate-decomposition.mlir | head -5Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look at the full test file to understand context
fd gate-decomposition.mlir -x catLength of output: 21283
🏁 Script executed:
#!/bin/bash
# Search for the decomposition algorithm implementation
rg -i "decompos" --type=cpp --type=h -n -B2 -A5 | head -100Length of output: 7525
🏁 Script executed:
#!/bin/bash
# Check if there are other tests with exact float constants in MLIR
rg -nP 'arith\.constant.*\d+\.\d{8,}' --type=mlir | wc -lLength of output: 30
🌐 Web query:
MLIR FileCheck floating-point constant testing best practices
💡 Result:
Brief best-practices for testing floating-point constants in MLIR FileCheck tests
-
Match the printer’s canonical form, not a hand-written decimal.
- MLIR prints floats in a canonical (usually scientific) form in many places — prefer matching that representation (see MLIR testing examples). [1][2]
-
Use regexes instead of literal numeric text.
- Avoid brittle exact-text checks like 0.1 or 0.100000 — use FileCheck regex groups such as {{[-+]?[0-9]+(.[0-9]+)?([eE][+-]?[0-9]+)?}} to accept integer, decimal and scientific forms. [3][4]
-
Capture and reuse constants with FileCheck variables when you care about identity.
- If you need to assert the same constant value or SSA name appears again, capture it (e.g. %[[C:.*]] or [[VAL:{{[-+]?[0-9]+(.[0-9]+)?([eE][+-]?[0-9]+)?}}]]) and reuse the variable later. This tests sameness without repeating fragile literals. [3]
-
Limit checks to what the test cares about (keep tests minimal).
- Only assert the constant token/attribute you need (use CHECK / CHECK-SAME / CHECK-NEXT sparingly). Don’t check unrelated printer details. [1]
-
When exact numeric formatting matters, document and assert the exact MLIR output.
- If the pass is explicitly required to format a float a certain way (e.g., a literal pass that normalizes to 1.000000e-01), assert that exact form but keep such tests narrowly scoped. Use comments to explain why exactness is required. [2]
-
For platforms/optimizations that may change rounding/printing, prefer tolerance in tests or move numeric correctness to execution-based tests.
- If small canonicalization/rounding differences can occur, either (a) make the FileCheck pattern accept a range/format using regex, or (b) write a lit/driver test that runs and numerically validates the value at runtime instead of relying on text output.
-
Useful FileCheck features to apply:
- {{...}} regex blocks, [[VAR:...]] capture/substitute, CHECK-SAME / CHECK-DAG to control ordering, and --enable-var-scope or -D for external variables when needed. [3][4]
References
- MLIR Testing Guide (FileCheck best practices). [1]
- MLIR Quickstart/Examples showing float attribute formatting (e.g., 1.000000e-01). [2]
- FileCheck documentation: regexes, variables and directives. [3][4]
Use FileCheck regexes and variables for floating-point constant matching instead of exact literals
The tests assert exact arith.constant f64 literals with 8–12 decimal places (e.g., 1.00543528660, 0.43624604946). This deviates from MLIR best practices and creates maintainability burden:
Per MLIR testing guidance, use regex patterns to match floating-point forms rather than exact decimal strings. For example, replace:
CHECK-DAG: %[[C0:.*]] = arith.constant 1.00543528660
with a regex that accepts integer, decimal, or scientific notation, then capture and reuse the value:CHECK-DAG: %[[C0:.*]] = arith.constant {{[-+]?[0-9]+(\.[0-9]+)?}}
Alternatively, capture constants and reuse them to test identity without repeating fragile literals. This ensures the test remains stable across compiler optimizations, platform differences, or changes to internal algorithms (e.g., Eigen), while still validating that the pass produces structurally correct decompositions.
Also implement testRepeatedDecomposition (lines 438–443)
The function contains only return despite its comment: "This test checks if the repeated application of the decomposition works by 'interrupting' the first series and then having a second one." Add the test logic described or remove the function if the scenario is not currently testable.
Applies to lines 280–347 (testTwoBasisGateDecomposition), 352–432 (testThreeBasisGateDecomposition), and other similar tests with exact constants.
There was a problem hiding this comment.
These tests need to be made platform-independent in general, because of floating-point precision issues the overall gate structure can change (different rotation gates with different values) - see the currently failing tests in e.g. the windows CI. @flowerthrower do you have an idea on how to test for equivalence of the overall pattern? My only idea would be to somehow integrate something that calculates the unitary matrix of an entire function and compare them in the end (but I'm unsure how to integrate that into the testing framework).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I would like to consult @burgholzer on this matter in a more general sense.
Given the fairly complex logic involved in the decomposition, I think it would be valuable to have a proper test of its execution. One more reason is that the decomposition does not involve MLIR at all.
In a standalone Google Test, standard gates with known unitary matrices could be decomposed independently of the MLIR infrastructure. The results could then be asserted against their expected parameters (e.g., K1, K2, ..., a, b, c, etc.), which can be inferred from Qiskit’s implementation or its tests. One could (and probably should) leverage the existing test infrastructure from the non-MLIR code for this. This may require slight adjustments to how the functions are exposed.
Conversely, the MLIR test could be relaxed somewhat—if it is known to be sensitive to floating-point errors—by using looser FileCheck patterns in the CHECK expressions.
There was a problem hiding this comment.
The logic is already checked by the assert statements in the code itself - if the decomposition is wrong, the program should crash. I agree that an external test for the final result of the Weyl decomposition could be nice, but I don't see how we can check the internal steps (and if it is worth the effort in this case). Should I add a couple of matrice decompositions to mlir/unittests?
There was a problem hiding this comment.
I have a couple of thoughts on this.
I agree with @flowerthrower that we can write tests for this part of the functionality that are independent from MLIR entirely. This might even justify moving the respective code outside of the MLIR subtree, but I probably wouldn't go as far.
Writing dedicated googletest-based unit tests seems to be a reasonable suggestion though that should allow us to write less brittle tests.
A great solution for equivalence checking would be our QCEC tool; but given how it builds directly on MQT Core itself, integrating it here (especially in C++) would be a mess.
Given how we are talking about 1- and 2- qubit gates only, it should be entirely feasible to write a very small script that multiplied the matrices and either does the equivalent of np.all_close() or computes tr(U^dagger V)/2^n if U and V are the two matrices to be compared and n is the number of qubits. Then, one would assert that the result is close enough to 1.
It would be good to come up with a couple of unit tests that assert the implementation produces sequences of gates we would expect and that this behaviour is maintained across updates.
I'll leave it at that for now. I'll probably have further comments when I actually look through the PR itself.
There was a problem hiding this comment.
Given how we are talking about 1- and 2- qubit gates only, it should be entirely feasible to write a very small script that multiplied the matrices and either does the equivalent of np.all_close() or computes tr(U^dagger V)/2^n if U and V are the two matrices to be compared and n is the number of qubits.
Just to make sure I understand you correctly: you want a Python script using pymlir to go through the unprocessed test file and then the output of this pass and compare them? I think that would be overkill and lots of double effort because it would require re-implementing all of the matrix stuff of C++ in python and I don't really want to do that because the new interface will (hopefully) make all that redundant anyway. Instead, I think we should maybe update the ToQuantumComputing pattern to accept allocQubit and use that for easy equivalence checking - we'll need to re-write that pattern anyway soon with the updated unitary interface.
Overall, I agree that it would be valuable to verify that the steps "from MLIR to matrix" and "gate sequence to MLIR" are correct, but I'm not really sure if it is worth the effort at the moment if all the unitary matrix logic of the dialect rewrite is still pending.
There was a problem hiding this comment.
No, nothing at all in Python (where did that come from? 🙃).
I did not mean script in the literal sense. I rather meant a function in C++ that does the respective check.
We have a very rough version of that in our C++ tests:
core/test/algorithms/eval_dynamic_circuits.cpp
Lines 130 to 183 in 2927385
As Patrick already mentioned, this kind of functionality can be tested entirely decoupled from MLIR as it is purely linear algebra based.
So you could simply generate a new googletest target in the mlir/unittests directory and place the respective tests there.
This should also allow you to write way better tests as you could with the FileCheck-based tests.
There was a problem hiding this comment.
No, nothing at all in Python (where did that come from? 🙃).
Oh sorry, I thought you meant a script in the literal sense because you also referred to numpy (although I now see you wrote "equivalent of np").
I'm already working on the unittests and re-structuring the code to extract the decompositions out of the pattern into header files for easier testing without including the source file. I thought you meant an additional test script to check the MLIR code, because otherwise something could go wrong in the translation from/to MLIR.
| module { | ||
| // CHECK-LABEL: func.func @testRepeatedDecomposition | ||
| func.func @testRepeatedDecomposition() { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
testRepeatedDecomposition is currently a no-op and doesn’t test the described scenario
The comment says this test should check “repeated application of the decomposition” with an interrupted first series followed by a second one, but the function body is just:
func.func @testRepeatedDecomposition() {
return
}So this test currently only verifies that running the pass on an empty function is a no-op.
Either:
- Flesh out the IR to match the described scenario (similar to the other tests), or
- Update the comment to reflect that this is intentionally a trivial regression/compilation test.
As written, it’s misleading and provides no additional coverage.
I'll verify this review comment by examining the actual file and surrounding context.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the file and examine lines around 438-443
if [ -f "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir" ]; then
echo "=== Lines 430-450 context ==="
sed -n '430,450p' "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir"
echo ""
echo "=== Grep for testRepeatedDecomposition and surrounding context ==="
grep -n -B5 -A10 "testRepeatedDecomposition" "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir"
else
echo "File not found"
fi
Length of output: 1211
🏁 Script executed:
#!/bin/bash
# Look for other test functions to understand the pattern
grep -n "func.func @test" "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir" | head -20Length of output: 1055
🏁 Script executed:
#!/bin/bash
# Check file line count and overall structure
wc -l "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir"Length of output: 63
🏁 Script executed:
#!/bin/bash
# Search for any comments related to "repeated" or this specific test
grep -n -i "repeated\|decomposition" "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir" | tail -20Length of output: 670
🏁 Script executed:
#!/bin/bash
# Get a broader view - examine the last 50 lines of the file
tail -50 "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir"Length of output: 2160
🏁 Script executed:
#!/bin/bash
# Look at one of the complete test functions as a comparison
grep -n -B2 -A15 "func.func @test" "mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir" | head -60Length of output: 2052
Add test IR to testRepeatedDecomposition or clarify that it's intentionally empty
Line 436 describes this test as checking "repeated application of the decomposition" with an "interrupted first series and then having a second one," but the function body (lines 440-442) contains only return. This mismatch means the test provides no coverage for the stated scenario. Either populate the function with appropriate IR to match the comment's description (following the pattern of other tests in this file), or update the comment to clarify this is a trivial regression test.
🤖 Prompt for AI Agents
In mlir/test/Dialect/MQTOpt/Transforms/gate-decomposition.mlir around lines 438
to 443, the test function @testRepeatedDecomposition is empty but the test
description says it should exercise "repeated application of the decomposition"
with an interrupted first series and a second one; either populate the func.func
body with MLIR that creates the interrupted-first-series-then-second-series
decomposition scenario (follow the style and opcode patterns used by neighboring
tests in this file so the decomposition pass has gates to rewrite), or update
the surrounding comment/description to explicitly state this is intentionally an
empty/trivial regression test; ensure whichever you choose keeps the CHECK lines
consistent with the new content.
flowerthrower
left a comment
flowerthrower
left a comment
There was a problem hiding this comment.
Thank you @taminob for all your work. The math and logic appear to be correctly implemented. I loosely checked the expected K1, K2, ... decomposition values with a small debug script on my Mac and was able to reproduce (roughtly) the same values as the Qiskit decomposition.
The three remaining points are:
- Set up a proper test to automate exactly this verification process.
- Improve comments — just because the Rust implementation is poorly documented does not mean we should do the same.
Especially since parts of your code are already nicely commented, it makes sense to extend this good style to the copied code. - Revisit MLIR logic after dialect revamp
| * gates. | ||
| * @param specialization Force the use this specialization. | ||
| */ | ||
| static TwoQubitWeylDecomposition create(const matrix4x4& unitaryMatrix, |
There was a problem hiding this comment.
Please, please, please: comments to guide humans through whats going on here.
There was a problem hiding this comment.
Sorry, I'll add some more - we really should be better than Qiskit here which was a pain to understand. Basically all the comments you see now were already added by me. :)
Did you perhaps understand the "re-order" step of the weyl coordinates? I already commented on what it does, but I have no idea why exactly this is necessary to be honest (although it also doesn't work without it).
There was a problem hiding this comment.
I also didn't fully understand that, but I guess it should be fine if it works.
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/GateDecompositionPattern.cpp
Outdated
Show resolved
Hide resolved
| module { | ||
| // CHECK-LABEL: func.func @testTwoBasisGateDecomposition | ||
| func.func @testTwoBasisGateDecomposition() { | ||
| // CHECK-DAG: %[[C0:.*]] = arith.constant -1.5707963267 | ||
| // CHECK-DAG: %[[C1:.*]] = arith.constant -3.141592653 | ||
| // CHECK-DAG: %[[C2:.*]] = arith.constant 1.2502369157 | ||
| // CHECK-DAG: %[[C3:.*]] = arith.constant 1.8299117708 | ||
| // CHECK-DAG: %[[C4:.*]] = arith.constant 2.8733113535 | ||
| // CHECK-DAG: %[[C5:.*]] = arith.constant 3.0097427712 | ||
| // CHECK-DAG: %[[C6:.*]] = arith.constant 0.2614370492 | ||
| // CHECK-DAG: %[[C7:.*]] = arith.constant -0.131849882 | ||
| // CHECK-DAG: %[[C8:.*]] = arith.constant 2.500000e+00 | ||
| // CHECK-DAG: %[[C9:.*]] = arith.constant -1.570796326 | ||
| // CHECK-DAG: %[[C10:.*]] = arith.constant 1.570796326 | ||
| // CHECK-DAG: %[[C11:.*]] = arith.constant 0.785398163 | ||
|
|
||
| // CHECK: %[[Q0_0:.*]] = mqtopt.allocQubit | ||
| // CHECK: %[[Q1_0:.*]] = mqtopt.allocQubit | ||
|
|
||
| // CHECK-NOT: mqtopt.gphase(%[[ANY:.*]]) | ||
| // CHECK: %[[Q0_1:.*]] = mqtopt.ry(%[[C11]]) %[[Q0_0]] | ||
| // CHECK: %[[Q0_2:.*]] = mqtopt.rz(%[[C10]]) %[[Q0_1]] | ||
| // CHECK: %[[Q1_1:.*]] = mqtopt.rx(%[[C9]]) %[[Q1_0]] | ||
| // CHECK: %[[Q1_2:.*]] = mqtopt.ry(%[[C8]]) %[[Q1_1]] | ||
| // CHECK: %[[Q0_3:.*]], %[[Q1_3:.*]] = mqtopt.x() %[[Q0_2]] ctrl %[[Q1_2]] | ||
| // CHECK: %[[Q0_4:.*]] = mqtopt.rz(%[[C7]]) %[[Q0_3]] | ||
| // CHECK: %[[Q0_5:.*]] = mqtopt.ry(%[[C6]]) %[[Q0_4]] | ||
| // CHECK: %[[Q0_6:.*]] = mqtopt.rz(%[[C5]]) %[[Q0_5]] | ||
| // CHECK: %[[Q1_4:.*]] = mqtopt.rz(%[[C10]]) %[[Q1_3]] | ||
| // CHECK: %[[Q1_5:.*]] = mqtopt.ry(%[[C10]]) %[[Q1_4]] | ||
| // CHECK: %[[Q0_7:.*]], %[[Q1_6:.*]] = mqtopt.x() %[[Q0_6]] ctrl %[[Q1_5]] | ||
| // CHECK: %[[Q0_8:.*]] = mqtopt.rz(%[[C4]]) %[[Q0_7]] | ||
| // CHECK: %[[Q0_9:.*]] = mqtopt.ry(%[[C3]]) %[[Q0_8]] | ||
| // CHECK: %[[Q0_10:.*]] = mqtopt.rz(%[[C2]]) %[[Q0_9]] | ||
| // CHECK: %[[Q1_7:.*]] = mqtopt.rz(%[[C1]]) %[[Q1_6]] | ||
| // CHECK: %[[Q1_8:.*]] = mqtopt.rx(%[[C0]]) %[[Q1_7]] | ||
|
|
||
| // ensure no other operations are inserted | ||
| // CHECK-NOT: mqtopt.[[ANY:.*]](%[[ANY:.*]]) | ||
|
|
||
| // CHECK: mqtopt.deallocQubit %[[Q0_10]] | ||
| // CHECK: mqtopt.deallocQubit %[[Q1_8]] | ||
|
|
||
| %cst0 = arith.constant 2.5 : f64 | ||
| %cst1 = arith.constant 1.2 : f64 | ||
| %cst2 = arith.constant 0.5 : f64 | ||
|
|
||
| %q0_0 = mqtopt.allocQubit | ||
| %q1_0 = mqtopt.allocQubit | ||
|
|
||
| %q0_x, %q1_x = mqtopt.i() %q0_0 ctrl %q1_0: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_1 = mqtopt.h() %q0_x: !mqtopt.Qubit | ||
| %q1_1, %q0_2 = mqtopt.x() %q1_x ctrl %q0_1: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_3, %q1_2 = mqtopt.rzz(%cst0) %q0_2, %q1_1: !mqtopt.Qubit, !mqtopt.Qubit | ||
| %q1_3 = mqtopt.ry(%cst1) %q1_2: !mqtopt.Qubit | ||
| %q0_4 = mqtopt.rx(%cst1) %q0_3: !mqtopt.Qubit | ||
| %q0_5, %q1_4 = mqtopt.x() %q0_4 ctrl %q1_3: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_6 = mqtopt.rz(%cst2) %q0_5: !mqtopt.Qubit | ||
| // make series longer to enforce decomposition | ||
| %q0_7, %q1_5 = mqtopt.i() %q0_6 ctrl %q1_4: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
| %q0_8, %q1_6 = mqtopt.i() %q0_7 ctrl %q1_5: !mqtopt.Qubit ctrl !mqtopt.Qubit | ||
|
|
||
| mqtopt.deallocQubit %q0_8 | ||
| mqtopt.deallocQubit %q1_6 | ||
|
|
||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
I would like to consult @burgholzer on this matter in a more general sense.
Given the fairly complex logic involved in the decomposition, I think it would be valuable to have a proper test of its execution. One more reason is that the decomposition does not involve MLIR at all.
In a standalone Google Test, standard gates with known unitary matrices could be decomposed independently of the MLIR infrastructure. The results could then be asserted against their expected parameters (e.g., K1, K2, ..., a, b, c, etc.), which can be inferred from Qiskit’s implementation or its tests. One could (and probably should) leverage the existing test infrastructure from the non-MLIR code for this. This may require slight adjustments to how the functions are exposed.
Conversely, the MLIR test could be relaxed somewhat—if it is known to be sensitive to floating-point errors—by using looser FileCheck patterns in the CHECK expressions.
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
2 times, most recently
from
7fc67db to
18cf6b0
Compare
|
I reworked the tests now (added unittests and made the MLIR tests more robust/less precise). A couple of questions:
|
I just skimmed through both the lit tests and the unit tests and I think the overall direction is fine. I am pretty sure that I will have some more detailed comments when I check the PR in more detail.
Hm. multiple thoughts on this.
These two might actually be more or less the same, the first might just be the second with some reasonable and relaxed defaults on the native gate-set.
I am not quite sure that I follow this. That link says:
We are using more modern compilers than this (by far). The current CI runs report
This seems to indicate to me that you might need to dig a little deeper there. An example of how that Attr is constructed would be This might not be final yet; mostly because I still want to explore some options that might allow us to make the gates/gate matrices singletons and avoid repeated constructions of matrices for gates where this is definitely not necessary.
I am not too familiar with the details of |
The documentation of I think it might be mainly about the stability of the interfaces - so it could happen that different Eigen versions will break API/ABI. However, since they are user-provided, I guess it also heavily depends on the actual function to be used (see also this stackoverflow question.
I'll look into it, although it's a bit hard on a different architecture. Could well be that it is caused by differences in alignment on ARM. |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
4 times, most recently
from
a9d1fc9 to
7efae47
Compare
Unfortunately, I couldn't really find the source of the error - also because I didn't manage to get a stacktrace in the CI and so can't really pinpoint the issue. I tried using C++23 I think the cause might be one of https://libeigen.gitlab.io/eigen/docs-nightly/group__TopicPassingByValue.html or https://libeigen.gitlab.io/eigen/docs-nightly/group__TopicStlContainers.html (which should be fixed since C++11, but maybe LLVM+MSVC behaves buggy here - who knows) which are quite serious and need to be considered when using Eigen (at least I wasn't aware of it). I'd suggest to simply set |
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
aee1e26 to
888deb2
Compare
llvm::numbers could be used instead
taminob
force-pushed
the
taminob/mlir-two-qubit-gate-decomposition-pass
branch
from
888deb2 to
7a80d30
Compare
Description
TODO
Related to #1122
Checklist: