-
-
Notifications
You must be signed in to change notification settings - Fork 47
Add multi-control functionality #1380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add CRZ, MCX, MCZ, MCRZ functionality to ZX package.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded multi-control ZX-diagram construction routines (addCcz, addCrz, addMcrz, addMcx, addMcz), integrated them into operation parsing and transformability checks, and expanded tests to validate multi-control constructions and post-reduction equivalence. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as parseOp
participant FC as FunctionalityConstruction
participant ZX as ZXDiagram
participant Anc as AncillaPool
Parser->>FC: receive qc::Operation (e.g., MCX / CRZ / RZ)
FC->>FC: select decomposition path (direct, CCZ, or multi-control)
alt requires ancilla
FC->>Anc: request ancilla qubits
Anc-->>FC: provide ancilla or return error
end
FC->>ZX: call addCcz/addCrz/addMcrz/addMcx/addMcz(...) to build ZX subgraph
ZX-->>Parser: updated ZXDiagram (operation represented)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zx/FunctionalityConstruction.cpp (1)
872-896: Remove unreachable return statement.The transformability checks correctly reflect the expanded multi-control support for RZ operations. However, line 896 contains an unreachable
return false;statement since the preceding conditions exhaust all cases.Apply this diff to remove the dead code:
case qc::OpType::RZ: return true; default: return false; } - return false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
include/mqt-core/zx/FunctionalityConstruction.hpp(1 hunks)src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(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.
📚 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:
test/zx/test_zx_functionality.cppinclude/mqt-core/zx/FunctionalityConstruction.hppsrc/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
include/mqt-core/zx/FunctionalityConstruction.hppsrc/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
src/zx/FunctionalityConstruction.cpp
🧬 Code graph analysis (2)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (2)
buildFunctionality(787-804)buildFunctionality(787-788)
src/zx/FunctionalityConstruction.cpp (2)
src/zx/Rational.cpp (1)
PiRational(21-45)src/ir/operations/OpType.cpp (2)
toString(21-35)toString(21-21)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/zx/FunctionalityConstruction.cpp
[warning] 388-388: src/zx/FunctionalityConstruction.cpp:388:13 [cppcoreguidelines-init-variables]
variable 'anc' is not initialized
[warning] 384-384: src/zx/FunctionalityConstruction.cpp:384:50 [clang-diagnostic-sign-conversion]
implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'difference_type' (aka 'long')
[warning] 384-384: src/zx/FunctionalityConstruction.cpp:384:50 [bugprone-narrowing-conversions]
narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:67 [clang-diagnostic-sign-conversion]
implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'difference_type' (aka 'long')
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:67 [bugprone-narrowing-conversions]
narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:5 [misc-const-correctness]
variable 'first' of type 'std::vector' (aka 'vector') can be declared 'const'
[warning] 382-382: src/zx/FunctionalityConstruction.cpp:382:5 [misc-const-correctness]
variable 'half' of type 'size_t' (aka 'unsigned long') can be declared 'const'
[warning] 356-356: src/zx/FunctionalityConstruction.cpp:356:5 [misc-const-correctness]
variable 'nextControl' of type 'Qubit' (aka 'int') can be declared 'const'
[warning] 339-339: src/zx/FunctionalityConstruction.cpp:339:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
[warning] 336-336: src/zx/FunctionalityConstruction.cpp:336:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
⏰ 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). (2)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (7)
include/mqt-core/zx/FunctionalityConstruction.hpp (1)
116-127: LGTM!The new method declarations for multi-control gates are well-structured and follow the existing API patterns. The signatures are clear and properly typed.
test/zx/test_zx_functionality.cpp (1)
232-359: Excellent test coverage for multi-control operations.The new tests thoroughly validate the multi-control functionality with:
- CRZ decomposition equivalence
- Multi-control X with varying control counts (0, 1, 3)
- Multi-control RZ with phase cancellation
- Edge cases for boundary conditions
All tests properly verify identity preservation, zero global phase, and connectivity after reduction.
src/zx/FunctionalityConstruction.cpp (5)
310-330: LGTM!The
addCczimplementation correctly decomposes the CCZ gate using the standard pattern with CNOTs, Z-spiders with appropriate phases, and Hadamard edges. The decomposition is consistent with ZX-calculus conventions.
332-340: LGTM!The
addCrzimplementation uses the correct decomposition for controlled-RZ gates. The argument order inaddCnot(diag, target, control, qubits)is intentional and follows the standard CRZ decomposition pattern where the roles are reversed.Note: The static analysis warning about swapped arguments at lines 336 and 339 is a false positive.
680-682: LGTM!The addition of the RZ case for single-control, single-target operations correctly routes to
addCrz, expanding the supported gate set.
723-740: LGTM!The updates to the two-control branch correctly:
- Build the controls vector from the operation
- Route CCZ operations to
addCcz- Route multi-controlled RZ operations to
addMcrz- Update error messages to reflect multi-control support
741-762: LGTM!The new multi-control branch properly handles operations with arbitrary numbers of controls and a single target. The routing to
addMcx,addMcz, andaddMcrzis correct and extends the ZX functionality to support general multi-controlled gates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zx/FunctionalityConstruction.cpp (1)
872-896: LGTM: Extended transformableToZX for controlled RZ.The updates correctly mark controlled-RZ operations as transformable in all control-count branches (1, 2, and N controls).
Minor cleanup - Line 896 has an unreachable return statement:
} else if (op->getNtargets() == 1) { switch (op->getType()) { case qc::OpType::X: case qc::OpType::Z: case qc::OpType::RZ: return true; default: return false; } - return false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
include/mqt-core/zx/FunctionalityConstruction.hpp(1 hunks)src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cppinclude/mqt-core/zx/FunctionalityConstruction.hpp
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
src/zx/FunctionalityConstruction.cpp
🧬 Code graph analysis (1)
src/zx/FunctionalityConstruction.cpp (1)
include/mqt-core/zx/FunctionalityConstruction.hpp (18)
diag(65-68)diag(69-72)diag(74-76)diag(77-78)diag(80-82)diag(83-85)diag(86-87)diag(88-89)diag(91-93)diag(95-97)diag(99-101)diag(102-103)diag(105-108)diag(110-113)diag(114-115)diag(116-117)op(60-60)op(135-135)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/zx/FunctionalityConstruction.cpp
[warning] 388-388: src/zx/FunctionalityConstruction.cpp:388:13 [cppcoreguidelines-init-variables]
variable 'anc' is not initialized
[warning] 384-384: src/zx/FunctionalityConstruction.cpp:384:50 [clang-diagnostic-sign-conversion]
implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'difference_type' (aka 'long')
[warning] 384-384: src/zx/FunctionalityConstruction.cpp:384:50 [bugprone-narrowing-conversions]
narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:67 [clang-diagnostic-sign-conversion]
implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'difference_type' (aka 'long')
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:67 [bugprone-narrowing-conversions]
narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined
[warning] 383-383: src/zx/FunctionalityConstruction.cpp:383:5 [misc-const-correctness]
variable 'first' of type 'std::vector' (aka 'vector') can be declared 'const'
[warning] 382-382: src/zx/FunctionalityConstruction.cpp:382:5 [misc-const-correctness]
variable 'half' of type 'size_t' (aka 'unsigned long') can be declared 'const'
[warning] 356-356: src/zx/FunctionalityConstruction.cpp:356:5 [misc-const-correctness]
variable 'nextControl' of type 'Qubit' (aka 'int') can be declared 'const'
[warning] 339-339: src/zx/FunctionalityConstruction.cpp:339:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
[warning] 336-336: src/zx/FunctionalityConstruction.cpp:336:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
⏰ 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). (2)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (7)
test/zx/test_zx_functionality.cpp (1)
232-359: Excellent test coverage for multi-control operations.The new tests comprehensively validate CRZ, MCX, and MCRZ operations by constructing equivalent circuits and verifying they reduce to identity. The test pattern is consistent with existing tests in the file.
src/zx/FunctionalityConstruction.cpp (5)
310-330: CCZ implementation looks correct.The addCcz function implements a double-controlled Z gate by adapting the CCX decomposition to work in the Z-basis.
332-340: CRZ implementation is correct.The addCrz function correctly implements controlled-RZ using the standard decomposition. The CNOT operations with target as control are intentional and correct for this decomposition, despite the static analysis warning.
680-682: LGTM: Single-controlled RZ support.Correctly handles controlled-RZ gates by delegating to the new addCrz function.
723-740: LGTM: Double-controlled gate support.The updated logic correctly handles CCZ and multi-controlled RZ operations with 2 controls by building a control vector and dispatching to the appropriate implementation functions.
741-762: LGTM: Multi-control gate support.The new branch properly handles gates with arbitrary numbers of controls, supporting MCX, MCZ, and MCRZ operations through the newly implemented functions.
include/mqt-core/zx/FunctionalityConstruction.hpp (1)
116-127: LGTM: Method declarations are well-structured.The new method declarations are consistent with existing patterns in the class and correctly match their implementations. Parameter types and const-correctness are appropriate.
Apply const to constant variables. Replace for loop with std::ranges::find_if implementation. Utilize static_cast to explicitly state conversion from unsigned to signed int. Shift return false of `transformableToZX` to be reachable again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/zx/FunctionalityConstruction.cpp(5 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cpp
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/zx/FunctionalityConstruction.cpp
[warning] 395-395: src/zx/FunctionalityConstruction.cpp:395:9 [readability-else-after-return]
do not use 'else' after 'throw'
[warning] 339-339: src/zx/FunctionalityConstruction.cpp:339:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
[warning] 336-336: src/zx/FunctionalityConstruction.cpp:336:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
⏰ 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 (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (9)
src/zx/FunctionalityConstruction.cpp (9)
310-330: LGTM!The
addCczimplementation correctly decomposes the CCZ gate using standard T-gate decomposition. The Hadamard edges are appropriately placed at the end (compared toaddCcxwhich has them at start/end) since CCZ = H·CCX·H on the target.
342-364: DeclarenextControlas const.The variable
nextControlis not modified after initialization and should be declaredconst.- const Qubit nextControl = controls.back(); + const Qubit nextControl = controls.back();Note: This appears to already be addressed in the current code (line 356 shows
const). If the static analysis tool is still flagging this, the issue may be resolved.
424-449: LGTM!The
addMczfunction correctly implements multi-controlled Z using recursive decomposition with proper base cases for 0, 1, and 2 controls. ThenextControlvariable is already declaredconst.
682-684: LGTM!Correctly routes single-controlled RZ operations to the new
addCrzfunction.
725-742: LGTM!The two-control parsing correctly handles CCZ and MCRZ operations. The controls vector is appropriately constructed for the MCRZ case while maintaining backward compatibility with the existing CCX/CCZ signatures.
743-760: LGTM!The multi-control parsing branch correctly handles MCX, MCZ (via H·MCX·H decomposition), and MCRZ operations for gates with more than 2 controls. The Hadamard sandwiching for MCZ is the correct identity.
874-897: LGTM!The
transformableToZXfunction correctly extends support to recognize RZ operations in single-control, two-control, and multi-control contexts, consistent with the new parsing implementations.
416-419: DeclarelastControlas const.The variable
lastControlis not modified after initialization.- const Qubit lastControl = controls.back(); + const Qubit lastControl = controls.back();Note: Line 416 already shows
constin the provided code, so this may already be addressed.
382-384: Consider declaringfirstas const.The
firstvector is not modified after construction and could be declaredconstfor better const-correctness. Thesecondvector cannot be const since it's modified on line 399.const auto half = static_cast<std::ptrdiff_t>((controls.size() + 1) / 2); - const std::vector<Qubit> first(controls.begin(), controls.begin() + half); + const std::vector<Qubit> first(controls.begin(), controls.begin() + half); std::vector<Qubit> second(controls.begin() + half, controls.end());Note: Line 383 already shows
constin the provided code, so this appears to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (2)
src/zx/FunctionalityConstruction.cpp (2)
buildFunctionality(789-806)buildFunctionality(789-790)test/zx/test_simplify.cpp (1)
fullReduce(602-602)
🪛 Cppcheck (2.18.0)
test/zx/test_zx_functionality.cpp
[error] 269-269: syntax error
(syntaxError)
🔇 Additional comments (4)
test/zx/test_zx_functionality.cpp (4)
222-245: CRZ decomposition test is mathematically soundThe CRZ(π/2) vs CX–RZ(-π/4)–CX–RZ(π/4) equivalence is correct (exact, no global phase), and the functionality + reduction checks are appropriate for validating the new CRZ support.
247-268: MultiCZ equivalence to H–MCX–H is correctUsing
qc.mcz({1, 2}, 0)and comparing againsth(0); mcx({1, 2}, 0); h(0);is the standard construction for MCZ and should be functionally identical with zero global phase; the identity and phase checks nicely exercise the new MCZ path.
269-295: CCZ vs H–MCX–H construction is appropriateImporting
cczfrom OpenQASM and comparing it withh(0); mcx({1, 2}, 0); h(0);is a valid way to test CCZ support; this pattern realizes a three-qubit phase flip on |111⟩ with no extra global phase, so the identity and zero-global-phase expectations are well chosen.
296-383: Good coverage of MCX semantics across arities and decompositionsThe new MCX tests collectively cover:
- general MCX construction and inversion (
MultiControlX);- ancilla-based decomposition on a larger register (
MultiControlXLarger);- the degenerate 0-control case behaving as plain X (
MultiControlX0);- the 1-control case matching CX (
MultiControlX1).These are well-structured functionality checks for the new multi-control-X support.
3142193 to
9dee2a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/zx/test_zx_functionality.cpp (1)
385-398: Consider strengthening connectivity assertions.The MCRZ test currently only checks connectivity for qubit 0. Following the pattern in
complexCircuit(lines 165-167), consider verifying connectivity for all involved qubits (0, 1, and 2) to catch potential wiring issues.Apply this diff to strengthen the test:
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); + EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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.
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cppsrc/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
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.
Applied to files:
test/zx/test_zx_functionality.cppsrc/zx/FunctionalityConstruction.cpp
📚 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:
test/zx/test_zx_functionality.cppsrc/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T23:16:16.276Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:16.276Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (1)
src/zx/FunctionalityConstruction.cpp (2)
buildFunctionality(791-808)buildFunctionality(791-792)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
test/zx/test_zx_functionality.cpp
[warning] 277-277: test/zx/test_zx_functionality.cpp:277:3 [misc-const-correctness]
variable 'diag' of type 'ZXDiagram' can be declared 'const'
src/zx/FunctionalityConstruction.cpp
[warning] 340-340: src/zx/FunctionalityConstruction.cpp:340:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
[warning] 337-337: src/zx/FunctionalityConstruction.cpp:337:3 [readability-suspicious-call-argument]
2nd argument 'target' (passed to 'ctrl') looks like it might be swapped with the 3rd, 'control' (passed to 'target')
⏰ 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). (4)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
🔇 Additional comments (15)
test/zx/test_zx_functionality.cpp (6)
222-245: LGTM: CRZ test validates controlled-RZ decomposition.The test correctly verifies that the CRZ gate implementation matches the expected decomposition using CNOT and RZ gates, and properly checks identity, zero global phase, and connectivity after reduction.
247-268: LGTM: MultiCZ test validates multi-controlled Z construction.The test correctly verifies that MCZ is equivalent to H-MCX-H construction, with proper identity, phase, and connectivity checks.
296-315: LGTM: Multi-control X now supported.The test correctly validates that multi-controlled X gates (with 3 controls) are now transformable and produce identity when inverted, confirming the PR objective to add multi-control support.
317-339: LGTM: Ancilla-assisted MCX decomposition validated.The test correctly verifies the ancilla-assisted decomposition of MCX with 3 controls in a 5-qubit system, ensuring the decomposition is functionally equivalent.
341-361: LGTM: Edge case for 0-control MCX validated.The test correctly verifies that MCX with no controls reduces to a simple X gate.
363-383: LGTM: Edge case for 1-control MCX validated.The test correctly verifies that MCX with one control reduces to a CNOT gate.
src/zx/FunctionalityConstruction.cpp (9)
310-330: LGTM: CCZ implementation optimizes away redundant Hadamard edges.The
addCczfunction correctly implements a double-controlled Z gate by removing the initial and final Hadamard transformations from the CCX decomposition, providing an optimized construction as mentioned in the PR description.
332-341: LGTM: CRZ decomposition correctly implemented.The controlled-RZ decomposition correctly uses reversed CNOT direction (target→control) with appropriate phase factors (±θ/2), and includes a clarifying comment per previous review feedback.
343-365: LGTM: MCRZ recursive decomposition is correct.The multi-controlled RZ implementation correctly handles:
- Base case 0: Direct RZ spider
- Base case 1: CRZ
- Recursive case: Standard decomposition using CRZ(θ/2) interleaved with MCX to implement the controlled phase gate
The recursive pattern matches standard quantum computing decompositions.
367-424: LGTM: MCX decomposition handles all cases correctly.The multi-controlled X implementation properly handles:
- Base cases: 0 controls (X), 1 control (CNOT), 2 controls (CCX)
- Ancilla-assisted recursion when spare qubits are available
- No-ancilla decomposition using RX + MCZ pattern when qubits are fully utilized
The ancilla search logic and error handling were addressed in previous reviews. The divide-and-conquer strategy is correct.
426-451: LGTM: MCZ recursive decomposition is correct.The multi-controlled Z implementation correctly handles base cases (0, 1, 2 controls) and uses the standard recursive decomposition with CRZ(π/2) interleaved with MCX for larger control counts.
684-686: LGTM: RZ added to single-control routing.Correctly routes controlled-RZ operations through the new
addCrzfunction when a single control is present.
727-744: LGTM: Two-control branch extended for Z and RZ.The two-control branch now:
- Extracts controls into a vector for use by multi-control functions
- Routes CCZ through
addCcz- Routes CCRZ through
addMcrzThis correctly extends the transformation capabilities for double-controlled gates.
745-766: LGTM: Multi-control branch handles X, Z, and RZ.The new multi-control branch (3+ controls, single target) correctly:
- Routes MCX through
addMcx- Routes MCZ through H-MCX-H pattern
- Routes MCRZ through
addMcrz- Throws for unsupported operations
This implements the core PR objective of supporting multi-controlled gates.
866-899: LGTM: transformableToZX correctly updated for RZ support.The
transformableToZXfunction now correctly recognizes RZ as transformable in:
- Single-control scenarios (line 876)
- Two-control scenarios (line 886)
- Multi-control scenarios (line 895)
This aligns with the new routing logic in
parseOpand fulfills the PR objective.
1d008c6 to
ccaf44d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/zx/FunctionalityConstruction.cpp (1)
332-341: About the suspiciousaddCrzargument order warningsStatic analysis flags the calls to
addCrzas “suspicious call argument” because the function signature isvoid FunctionalityConstruction::addCrz( ZXDiagram& diag, const PiExpression& phase, const Qubit target, const Qubit control, std::vector<Vertex>& qubits)while call sites conceptually pass
(control, target).Given the body:
// CRZ decomposition uses reversed CNOT direction addCnot(diag, control, target, qubits); addZSpider(diag, target, qubits, -phase / 2); addZSpider(diag, control, qubits, phase / 2); addCnot(diag, control, target, qubits);and how it’s invoked from
parseOpand the multi-control helpers, the effective semantics are consistent with the intended CRZ decomposition (as validated by the new CRZ / MCRZ tests). The warnings are therefore a readability issue rather than a correctness bug.If you want to silence the tools and make the intent clearer, consider flipping the parameter names/order to
(const Qubit control, const Qubit target)and updating the call sites accordingly, so callers and implementation agree on ordering and static analysis no longer complains.Also applies to: 360-363, 446-448, 684-686
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
📚 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:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T23:58:03.684Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:03.684Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T23:16:16.276Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:16.276Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
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.
Applied to files:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (2)
buildFunctionality(791-808)buildFunctionality(791-792)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/zx/FunctionalityConstruction.cpp
[warning] 448-448: src/zx/FunctionalityConstruction.cpp:448:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 446-446: src/zx/FunctionalityConstruction.cpp:446:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 362-362: src/zx/FunctionalityConstruction.cpp:362:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 360-360: src/zx/FunctionalityConstruction.cpp:360:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 685-685: src/zx/FunctionalityConstruction.cpp:685:7 [readability-suspicious-call-argument]
3rd argument 'ctrl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
⏰ 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). (4)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
🔇 Additional comments (3)
test/zx/test_zx_functionality.cpp (1)
222-505: New multi-control ZX tests look correct and give solid coverageThe CRZ, CCZ, MCX/MCZ, and MCRZ tests follow the existing testing pattern (build both circuits, compose with inverse, reduce, then check identity/global phase/connectivity) and cover:
- 0/1/2/many controls for X, Z, and RZ;
- both direct constructions (mcz/mcx/mcrz/crz) and decompositions via standard gates (CX, H, RZ).
I don’t see functional or structural issues here; these should give good end‑to‑end coverage of the new functionality.
src/zx/FunctionalityConstruction.cpp (2)
310-365: Multi-control CCZ / CRZ / MCRZ / MCZ decompositions look structurally soundThe new helpers
addCcz(two controls, Z-like),addCrz(controlled-RZ),addMcrz(multi-controlled RZ, with 0/1-control base cases and recursive step),addMcz(multi-controlled Z with 0/1/2-control base cases and recursive step),are consistent with known constructions:
- Base cases (0 controls → single-qubit gate, 1 control → CRZ, 2 controls → CCZ) are explicit.
- Recursive branches always shrink
controls(viacontrols.pop_back()), so the recursion terminates.- The CRZ helper is reused in both the single-control path (in
parseOp) and the multi-control decompositions, keeping the semantics aligned.- Tests exercise CRZ, CCZ, MCRZ (including k=0/1 and k>1), and MCZ (including k=0/1/2 and larger), and also check global-phase cancellation.
Given that and the existing ZX primitives, the design here looks correct and maintainable.
Also applies to: 426-451
661-767:parseOpandtransformableToZXextensions for controlled RZ and multi-control gates are consistentThe changes in:
parseOp:
- add a
qc::OpType::RZcase for 1-control/1-target gates usingaddCrz;- route 2-control RZ to
addMcrzwith a 2-element controls vector;- route general multi-control X/Z/RZ (one target) to
addMcx, the CZ-styleH–MCX–Hpattern, andaddMcrzrespectively.
transformableToZX:
- include
qc::OpType::RZin the supported sets for
- uncontrolled single-qubit,
- single-control two-qubit, and
- multi-control single-target operations,
are aligned: every newly-routed case has a corresponding “supported” entry, and unsupported multi-control types still fall through to the existing exceptions (as exercised by the
UnsupportedControltests).I don’t see missing or inconsistent cases here.
Also applies to: 876-899
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/zx/FunctionalityConstruction.cpp (1)
332-341: Consider swapping parameter names to match codebase convention.The parameter order
(target, control)differs from the convention used elsewhere in this file (e.g.,addCnot,addCphaseuse(ctrl, target)). This triggers persistent static analysis warnings at every call site (lines 354, 360, 362, 436, 446, 448, 685).While the comment clarifies the decomposition intent, renaming the parameters would eliminate linter noise and improve readability.
void FunctionalityConstruction::addCrz(ZXDiagram& diag, const PiExpression& phase, - const Qubit target, const Qubit control, + const Qubit ctrl, const Qubit target, std::vector<Vertex>& qubits) { // CRZ decomposition uses reversed CNOT direction - addCnot(diag, control, target, qubits); - addZSpider(diag, target, qubits, -phase / 2); - addZSpider(diag, control, qubits, phase / 2); - addCnot(diag, control, target, qubits); + addCnot(diag, target, ctrl, qubits); + addZSpider(diag, ctrl, qubits, -phase / 2); + addZSpider(diag, target, qubits, phase / 2); + addCnot(diag, target, ctrl, qubits); }This would require updating all call sites to swap arguments accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cppsrc/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cppsrc/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T23:58:03.684Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:03.684Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T23:16:16.276Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:16.276Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
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.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpp
🧬 Code graph analysis (2)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (2)
buildFunctionality(791-808)buildFunctionality(791-792)
src/zx/FunctionalityConstruction.cpp (2)
include/mqt-core/ir/operations/OpType.hpp (1)
toString(40-40)src/ir/operations/OpType.cpp (2)
toString(21-35)toString(21-21)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/zx/FunctionalityConstruction.cpp
[warning] 448-448: src/zx/FunctionalityConstruction.cpp:448:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 446-446: src/zx/FunctionalityConstruction.cpp:446:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 362-362: src/zx/FunctionalityConstruction.cpp:362:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 360-360: src/zx/FunctionalityConstruction.cpp:360:5 [readability-suspicious-call-argument]
3rd argument 'nextControl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
[warning] 685-685: src/zx/FunctionalityConstruction.cpp:685:7 [readability-suspicious-call-argument]
3rd argument 'ctrl' (passed to 'target') looks like it might be swapped with the 4th, 'target' (passed to 'control')
⏰ 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). (2)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
🔇 Additional comments (13)
src/zx/FunctionalityConstruction.cpp (6)
310-330: LGTM: CCZ decomposition correctly optimizes away Hadamard bookends.The
addCczimplementation is a well-optimized variant of CCX that directly produces the CCZ decomposition. Compared toaddCcx, it removes the opening H gate and adjusts the closing sequence to use an H-edge followed by another H-edge, which correctly implements CCZ = H(target) · CCX · H(target) in a more efficient ZX form.
343-365: LGTM: MCRZ recursive decomposition is correct.The implementation correctly handles base cases (0 and 1 controls) and uses the standard recursive decomposition: CRZ(θ/2) · MCX · CRZ(-θ/2) · MCX for extracting one control at a time.
367-424: LGTM: MCX decomposition with ancilla-based optimization.The implementation correctly handles all base cases and provides two decomposition paths:
- When ancilla qubits are available: efficient divide-and-conquer using the spare qubit
- When no ancilla available: RX-based decomposition using MCZ and MCRZ
Previous review issues (uninitialized
anc, missing error check) have been properly addressed.
426-451: LGTM: MCZ recursive decomposition is correct.The implementation properly handles all base cases including the use of
addCczfor 2 controls (which is more efficient than the general recursion). The recursive decomposition follows the standard pattern.
684-686: LGTM: parseOp correctly routes multi-control operations to new handlers.The integration properly:
- Routes single-control RZ to
addCrz- Builds control vectors from operation controls for 2+ control cases
- Delegates to
addMcrz,addMcx,addMczas appropriateThe static analysis warning on line 685 is the same
addCrzparameter naming issue noted earlier.Also applies to: 727-744, 745-766
876-900: LGTM: transformableToZX correctly updated for new multi-control gates.The transformability checks now properly reflect the expanded parseOp capabilities, allowing RZ in single-control, 2-control, and arbitrary multi-control contexts.
test/zx/test_zx_functionality.cpp (7)
222-245: LGTM: CRZ test properly validates against known decomposition.The test correctly validates CRZ by comparing against the explicit decomposition: CNOT · RZ(-θ/2) · CNOT · RZ(θ/2).
247-268: LGTM: MultiCZ test validates against H·MCX·H equivalence.Correct validation that MCZ equals H·MCX·H on the target qubit.
269-293: LGTM: CCZ test validates correctly.Past review issue (unused
diagvariable) has been addressed.
316-338: LGTM: Proper validation against ancilla-based decomposition.This test correctly validates the MCX decomposition by comparing against the explicit ancilla-based construction that the implementation uses internally.
340-382: LGTM: Boundary cases for MCX properly tested.
MultiControlX0validates 0-control MCX equals X, andMultiControlX1validates 1-control MCX equals CNOT. These are important boundary condition tests.
384-449: LGTM: Comprehensive MCZ test coverage.The tests properly validate:
MultiControlZ: MCZ equals H·MCX·HMultiControlZ0: 0-control MCZ equals ZMultiControlZ1: 1-control MCZ equals CZGood coverage of boundary conditions and correct equivalences.
451-506: LGTM: MCRZ tests cover self-inverse and boundary cases.The tests validate:
MultiControlRZ: MCRZ(θ) · MCRZ(-θ) = I (self-inverse property)MultiControlRZ0: 0-control MCRZ equals RZMultiControlRZ1: 1-control MCRZ equals CRZGood boundary condition coverage.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Marcel Walter <[email protected]>
addMcz and format
Add test for MultiControlZ with 3 controls. Add transformabletoZX tests for MCZ, MCX, MCrz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (8)
test/zx/test_zx_functionality.cpp (7)
222-246: Add connectivity assertion for qubit 1.This 2-qubit test only validates connectivity for qubit 0 after reduction. Add a check for qubit 1 to ensure both wires are properly connected.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); }
248-269: Add connectivity assertions for all qubits.This 3-qubit test only validates connectivity for qubit 0. Add checks for qubits 1 and 2.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); + EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); }
270-294: Add connectivity assertions for all qubits.This 3-qubit CCZ test only validates connectivity for qubit 0. Add checks for qubits 1 and 2.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); + EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); }
296-316: Add connectivity assertions for all qubits.This 4-qubit test only validates connectivity for qubit 0. Add checks for qubits 1-3.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); + EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); + EXPECT_TRUE(d.connected(d.getInput(3), d.getOutput(3))); }
365-385: Add connectivity assertion for qubit 1.This 2-qubit test only validates connectivity for qubit 0. Add a check for qubit 1.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); }
387-409: Add connectivity assertion for qubit 3.This 4-qubit test checks connectivity for qubits 0-2 but misses qubit 3.
EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); + EXPECT_TRUE(d.connected(d.getInput(3), d.getOutput(3))); }
478-492: Add connectivity assertions for qubits 1 and 2.This 3-qubit test only validates connectivity for qubit 0. Add checks for qubits 1 and 2.
EXPECT_TRUE(d.isIdentity()); EXPECT_TRUE(d.globalPhaseIsZero()); EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); + EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); + EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))); }src/zx/FunctionalityConstruction.cpp (1)
705-722: Consider constructing controls only when needed.The
controlsvector is built unconditionally but only used for the RZ case. For X and Z,ctrl0andctrl1are used directly. This introduces slight redundancy.Consider moving the controls construction into the RZ case:
switch (op->getType()) { case qc::OpType::X: addCcx(diag, ctrl0, ctrl1, target, qubits); break; case qc::OpType::Z: addCcz(diag, ctrl0, ctrl1, target, qubits); break; - case qc::OpType::RZ: + case qc::OpType::RZ: { + std::vector<Qubit> controls; + for (const auto& ctrl : op->getControls()) { + controls.push_back(static_cast<Qubit>(p.at(ctrl.qubit))); + } addMcrz(diag, parseParam(op.get(), 0), controls, target, qubits); break; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/zx/FunctionalityConstruction.cpp(5 hunks)test/zx/test_zx_functionality.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 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:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T23:58:09.648Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-08T23:16:20.680Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-14T17:02:02.997Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
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.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T14:55:43.899Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
src/zx/FunctionalityConstruction.cpp
📚 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:
src/zx/FunctionalityConstruction.cpptest/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (2)
src/zx/FunctionalityConstruction.cpp (10)
include/mqt-core/zx/FunctionalityConstruction.hpp (18)
diag(65-68)diag(69-72)diag(74-76)diag(77-78)diag(80-82)diag(83-85)diag(86-87)diag(88-89)diag(91-93)diag(95-97)diag(99-101)diag(102-103)diag(105-108)diag(110-113)diag(114-115)diag(116-117)op(60-60)op(135-135)include/mqt-core/ir/QuantumComputation.hpp (13)
target(333-333)target(337-337)qubits(320-320)qubits(522-522)first(562-564)first(562-562)op(96-96)op(348-350)op(351-354)op(355-358)op(359-360)op(361-363)op(364-366)src/zx/Rational.cpp (1)
PiRational(21-45)include/mqt-core/zx/ZXDiagram.hpp (1)
phase(423-423)include/mqt-core/ir/operations/Operation.hpp (8)
controls(67-67)op(51-51)op(51-51)op(52-52)op(52-52)op(184-186)p(127-127)p(127-127)include/mqt-core/ir/operations/CompoundOperation.hpp (2)
first(166-168)first(166-166)include/mqt-core/zx/ZXDefinitions.hpp (2)
ZXException(83-84)ZXException(83-83)include/mqt-core/dd/Package.hpp (3)
p(1783-1785)p(1787-1789)p(1790-1792)include/mqt-core/ir/operations/OpType.hpp (1)
toString(40-40)src/ir/operations/OpType.cpp (2)
toString(21-35)toString(21-21)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (6)
buildFunctionality(767-784)buildFunctionality(767-768)transformableToZX(785-789)transformableToZX(785-786)transformableToZX(791-878)transformableToZX(791-791)
⏰ 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). (4)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
🔇 Additional comments (6)
src/zx/FunctionalityConstruction.cpp (6)
310-330: CCZ implementation looks correct.The CCZ implementation correctly optimizes away one Hadamard edge compared to
H·CCX·H. The tests verify correctness by comparing against theH·MCX·Hdecomposition.
332-341: CRZ implementation is correct.The comment clarifies the intentional reversal of CNOT direction for the CRZ decomposition.
358-416: MCX implementation is correct.The implementation properly handles base cases (0, 1, 2 controls) and uses a recursive decomposition for larger cases. The ancilla selection now correctly iterates over wire indices rather than vertex values, addressing the previous concern about out-of-bounds access.
662-664: CRZ parsing is correct.The CRZ case properly delegates to
addCrzfor single-control single-target RZ operations.
723-742: Multi-control single-target handling is correct.The new branch properly handles MCX, MCZ, and MCRZ for operations with more than 2 controls.
852-875: transformableToZX updates are correct.The function now properly recognizes RZ in single-control, two-control, and multi-control contexts, aligning with the new CRZ and MCRZ capabilities.
35e1924 to
9de9ac9
Compare
…e target for controlled operations
|
Hey @keefehuang 👋🏼 May I ask what the status is here? There still seem to be some unresolved comments from the CodeRabbit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @test/zx/test_zx_functionality.cpp:
- Around line 249-270: The test MultiCZ currently checks connectivity only for
qubit 0; update the assertions in TEST_F(ZXFunctionalityTest, MultiCZ) to also
assert connectivity for qubits 1 and 2 by adding
EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))) and
EXPECT_TRUE(d.connected(d.getInput(2), d.getOutput(2))) after the existing
connectivity check so all three qubit input-output pairs are validated.
- Around line 271-295: The test ZXFunctionalityTest::CCZ only asserts
input-output connectivity for qubit 0; add connectivity checks for qubits 1 and
2 by calling d.connected(d.getInput(1), d.getOutput(1)) and
d.connected(d.getInput(2), d.getOutput(2)) as EXPECT_TRUE assertions after the
existing connectivity check so the 3-qubit circuit is fully validated; locate
these additions in the TEST_F(ZXFunctionalityTest, CCZ) test after the existing
EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))) line.
- Around line 463-487: The test allocates 4 qubits though only qubits 0–2 are
used; change the QuantumComputation constructors for qc and qcPrime from 4 to 3
(in TEST_F(ZXFunctionalityTest, MultiControlZ2)) so mcz({1, 2}, 0) and the
H/MCX/H sequence remain valid, then remove the redundant connectivity assertion
EXPECT_TRUE(d.connected(d.getInput(3), d.getOutput(3))); so the test only checks
connectivity for qubits 0–2 after buildFunctionality, d.concat(dPrime.invert()),
and fullReduce.
- Around line 373-393: The test TEST_F(ZXFunctionalityTest, MultiControlX1) only
asserts connectivity for qubit 0; add a connectivity assertion for qubit 1 by
calling d.connected(d.getInput(1), d.getOutput(1)) after the existing checks on
d (after fullReduce and alongside the other EXPECT_TRUE checks) so both qubit
inputs/outputs (via d.getInput(1) and d.getOutput(1)) are verified.
- Around line 489-503: The test ZXFunctionalityTest::MultiControlRZ only asserts
connectivity for qubit 0; add assertions for qubits 1 and 2 by calling
d.connected(d.getInput(1), d.getOutput(1)) and d.connected(d.getInput(2),
d.getOutput(2)) after building and reducing the functionality (after
fullReduce(d)), so the test verifies input-output connectivity for all three
qubits in the qc/mcrz circuit.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/zx/test_zx_functionality.cpp
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 🐍 Test (windows-2025) / 🐍 windows-2025
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Release) / 🏁 windows-2025 msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (1)
test/zx/test_zx_functionality.cpp (1)
526-545: Incomplete connectivity checks.The test creates a 2-qubit circuit but only verifies input-output connectivity for qubit 0 (line 544). For comprehensive validation, add a connectivity assertion for qubit 1:
EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1)));⛔ Skipped due to learnings
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.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/zx/test_zx_functionality.cpp:
- Around line 222-552: Many TEST_F cases (e.g., CRZ, MultiCZ, CCZ,
MultiControlX*, MultiControlZ*, MultiControlRZ*) duplicate the same pattern;
refactor into a parameterized test by defining a struct like EquivalenceTestCase
with fields name, buildCircuit and buildEquivalent
(std::function<void(qc::QuantumComputation&)>), and nQubits, create a test
fixture class ZXEquivalenceTest : public
::testing::TestWithParam<EquivalenceTestCase>, implement
TEST_P(ZXEquivalenceTest, CircuitsAreEquivalent) to run the shared logic
(construct qc via qc::QuantumComputation, call params.buildCircuit and
params.buildEquivalent, build functionalities via
FunctionalityConstruction::buildFunctionality, concat/invert/fullReduce and
ASSERT expectations), then register cases with INSTANTIATE_TEST_SUITE_P using
descriptive names so each former TEST_F case becomes an entry in the
instantiation list.
- Around line 533-552: Add the missing connectivity assertion for qubit 1 in the
MultiControlRZ1 test: after the existing checks on d.isIdentity(),
d.globalPhaseIsZero(), and connected(d.getInput(0), d.getOutput(0)), also assert
connected(d.getInput(1), d.getOutput(1)) so both qubits are verified like the
other 2-qubit tests.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/zx/test_zx_functionality.cpp
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (6)
buildFunctionality(767-784)buildFunctionality(767-768)transformableToZX(785-789)transformableToZX(785-786)transformableToZX(791-868)transformableToZX(791-791)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Debug) / 🍎 macos-15 clang Debug
- GitHub Check: 🇨 Coverage / 📈 Coverage
- GitHub Check: 🐍 Lint / 🚨 Lint
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (4)
test/zx/test_zx_functionality.cpp (4)
222-247: LGTM! Comprehensive CRZ validation.The test correctly validates the CRZ gate by comparing it against a decomposition using CX and RZ gates, and properly checks transformability, identity, global phase, and connectivity for all qubits.
249-299: LGTM! Good coverage of MCZ variations.The tests validate multi-controlled Z gates through both direct API calls and QASM parsing, correctly verifying the H-MCX-H equivalence. The dual approach provides good coverage.
301-398: LGTM! Thorough MCX test coverage.The test suite comprehensively validates multi-controlled X gates:
- Self-inverse property for general case
- Ancilla-based decomposition for larger gates
- Edge cases (0 and 1 controls reduce to X and CX)
All connectivity assertions are present and correct.
400-492: LGTM! Complete MCZ test suite.The tests validate multi-controlled Z gates across all control counts:
- General multi-control case (self-inverse property)
- Edge cases reducing to Z and CZ
- MultiControlZ2 correctly tests with an unused qubit, verifying connectivity preservation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/zx/test_zx_functionality.cpp:
- Around line 273-275: In TEST_F(ZXFunctionalityTest, CCZ) remove the unused
using directive "using namespace qc::literals;" because the test loads the
circuit from QASM and does not reference any qc::literals symbols; simply delete
that line from the test function (leave the rest of the function and the const
std::string testfile = "OPENQASM 2.0;" intact).
- Around line 249-272: The test MultiCZ is missing the transformability checks;
before building/inverting the ZX functionalities, call
FunctionalityConstruction::transformableToZX(&qc) and
FunctionalityConstruction::transformableToZX(&qcPrime) (or the equivalent
transformableToZX check used in other tests) and assert they return true to
ensure consistency with other multi-control tests; place these EXPECT_TRUE
checks before the calls to FunctionalityConstruction::buildFunctionality so the
test verifies both circuits are transformable to ZX prior to concatenation and
reduction.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/zx/test_zx_functionality.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
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.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (6)
buildFunctionality(767-784)buildFunctionality(767-768)transformableToZX(785-789)transformableToZX(785-786)transformableToZX(791-868)transformableToZX(791-791)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2025) / 🐍 windows-2025
- GitHub Check: 🐍 Test (macos-15) / 🐍 macos-15
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Release) / 🏁 windows-2025 msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Release) / 🍎 macos-15 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Lint / 🚨 Lint
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (8)
test/zx/test_zx_functionality.cpp (8)
222-247: LGTM! Test correctly verifies CRZ decomposition.The test properly validates CRZ against its known decomposition (CX-RZ-CX-RZ sequence). Consider moving the
transformableToZXcheck (line 242) beforebuildFunctionality(line 234) to match the pattern used in other tests and catch unsupported operations early.
301-324: Test verifies self-consistency rather than correctness against a reference decomposition.Both
qcandqcPrimecontain identicalmcx({1, 2, 3}, 0)operations. This test confirms thatMCX * MCX⁻¹ = I(i.e., the ZX construction and inversion are consistent), but doesn't validate the implementation against an independent reference. TheMultiControlXLargertest (lines 326-353) does provide such verification using an ancilla-based decomposition.Consider either:
- Keeping as-is if the intent is to test construction/inversion consistency
- Replacing
qcPrimewith a reference decomposition for true equivalence testing
355-375: LGTM! Good edge case coverage.Correctly tests that MCX with zero controls degenerates to a simple X gate.
377-398: LGTM! Good edge case coverage.Correctly tests that MCX with one control is equivalent to CX.
400-423: Same observation asMultiControlX: tests self-consistency.Both circuits contain identical
mcz({1, 2, 3}, 0). This validates construction/inversion consistency but not against an independent reference decomposition. Consider using the H-MCX-H decomposition pattern shown in theMultiCZandMultiControlZ2tests for true equivalence verification.
425-492: LGTM! Comprehensive edge case coverage for MCZ.The tests correctly verify:
- Zero controls degenerates to Z gate
- One control is equivalent to CZ
- Two controls matches H-MCX-H decomposition
The use of 4 qubits in
MultiControlZ2while only using 3 appropriately tests that unused qubits pass through correctly.
494-553: LGTM! Good coverage for MCRZ functionality.The tests properly verify:
- MCRZ rotation cancellation (positive then negative angle)
- Zero controls degenerates to RZ
- One control is equivalent to CRZ
Consistent use of
transformableToZXchecks.
326-353: The 4-gate MCX decomposition pattern in the test is correct and intentionally implemented.The observation that the fourth
MCX({3, 4}, 0)appears redundant is technically accurate—after line 334 (the uncompute step), qubit 4 is reset to |0⟩, so the gate's condition can never be satisfied. However, this pattern is explicitly implemented insrc/zx/FunctionalityConstruction.cpp:395-398and the test validates that the 4-gate sequence is functionally equivalent toMCX({1,2,3}, 0)by building ZX diagrams for both, concatenating with the inverse, and reducing to identity. ThefullReduce()step confirms functional equivalence, so either this pattern is a deliberate (if non-obvious) implementation choice or the redundant gate is successfully eliminated during reduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @test/zx/test_zx_functionality.cpp:
- Around line 467-491: The test MultiControlZ2 in ZXFunctionalityTest allocates
4 qubits but only uses qubits 0,1,2; change the QuantumComputation constructions
qc and qcPrime to use 3 qubits instead of 4 so the test matches intent and other
tests (e.g., MultiCZ, CCZ); update the calls qc = qc::QuantumComputation(4) and
auto qcPrime = qc::QuantumComputation(4) to use 3, leaving the rest of the test
(mcz, h, mcx, FunctionalityConstruction::buildFunctionality, d.concat,
fullReduce and the EXPECT_* checks) unchanged.
- Around line 222-247: Add an assertion that the constructed QuantumComputation
is transformable to ZX in each test missing it by inserting
EXPECT_TRUE(FunctionalityConstruction::transformableToZX(&qc)); immediately
after constructing/configuring qc (and before calling
FunctionalityConstruction::buildFunctionality(&qc)). Apply this change to the
tests referenced (e.g., TEST_F(ZXFunctionalityTest, MultiControlX), TEST_F(...,
MultiControlZ), TEST_F(..., MultiControlRZ) and the test blocks covering the
ranges 249-272, 354-374, 376-397, 424-443, 445-465, 511-530, 532-552), using the
same FunctionalityConstruction::transformableToZX call as in existing tests like
CRZ to ensure consistency.
- Around line 300-323: The test MultiControlX currently compares an MCX circuit
to an identical copy, which is tautological; instead construct qcPrime as a
known decomposition of MCX (e.g., H on target, equivalent multi-controlled Z
construction, then H on target) and compare the ZX functionality of qc against
that decomposition: locate ZXFunctionalityTest::MultiControlX and replace the
qcPrime construction so it builds the H-MCZ-H decomposition (apply
qcPrime.h(target), the multi-control Z circuit used in
MultiControlZ2/MultiControlXLarger tests, then qcPrime.h(target)), then call
FunctionalityConstruction::buildFunctionality on both, concat and fullReduce as
before, and keep the same EXPECT_* assertions to validate equivalence.
- Around line 399-422: The test MultiControlZ currently compares qc.mcz({1, 2,
3}, 0) to an identical qcPrime, which is tautological; replace qcPrime with an
H-MCX-H decomposition as the reference: create qcPrime by applying H to qubit 0,
then qcPrime.mcx({1,2,3}, 0) (or mcx equivalent), then H on qubit 0 again, so
the constructed ZX from qcPrime matches the expected MCZ; keep using
FunctionalityConstruction::buildFunctionality, d.concat(dPrime.invert()),
fullReduce(d) and the same EXPECT_TRUE checks to verify identity, global phase
and connectivity.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/zx/test_zx_functionality.cpp
🧰 Additional context used
🧠 Learnings (12)
📓 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.
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 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:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-28T17:05:10.588Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1403
File: test/qir/runner/test_qir_runner.cpp:35-35
Timestamp: 2025-12-28T17:05:10.588Z
Learning: In the munich-quantum-toolkit/core repository, Windows Unicode support (via _wsystem) is not needed for test utilities like test/qir/runner/test_qir_runner.cpp. Using std::system is acceptable for system calls in tests.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-22T01:25:21.609Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/ir/register_permutation.cpp:153-171
Timestamp: 2025-12-22T01:25:21.609Z
Learning: In the munich-quantum-toolkit/core repository, when using nanobind iterator factory functions like `make_key_iterator` and `make_iterator`, the unqualified form (without explicit `nb::` prefix) is preferred. The clang-tidy configuration suggests removal of explicit namespace qualification, relying on ADL (Argument-Dependent Lookup) to resolve these functions correctly.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-08T23:58:09.648Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
test/zx/test_zx_functionality.cpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/zx/test_zx_functionality.cpp
🧬 Code graph analysis (1)
test/zx/test_zx_functionality.cpp (2)
include/mqt-core/zx/FunctionalityConstruction.hpp (2)
qc(39-39)qc(50-50)src/zx/FunctionalityConstruction.cpp (6)
buildFunctionality(767-784)buildFunctionality(767-768)transformableToZX(785-789)transformableToZX(785-786)transformableToZX(791-868)transformableToZX(791-791)
🔇 Additional comments (6)
test/zx/test_zx_functionality.cpp (6)
222-247: LGTM!The CRZ test correctly validates the controlled-RZ implementation by comparing against the standard CX-RZ-CX-RZ decomposition. The assertions for identity, zero global phase, and connectivity are appropriate.
249-272: LGTM!The MultiCZ test correctly validates MCZ by comparing against the H-MCX-H decomposition.
273-298: LGTM!The CCZ test correctly validates the CCZ gate loaded via QASM against the expected H-MCX-H decomposition.
325-352: LGTM!The MultiControlXLarger test provides meaningful validation by comparing a 3-control MCX against its ancilla-based decomposition using 4 MCX gates with 2 controls each.
354-397: LGTM!The edge case tests for MCX with 0 controls (reducing to X) and 1 control (reducing to CX) are correctly implemented and provide good boundary condition coverage.
493-552: LGTM!The MCRZ tests provide good coverage including the self-inverse check and edge cases for 0 and 1 control qubits.
|
Apologies, just back from vacation. All comments have been addressed. |
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @keefehuang for this contribution! This is great to see. 🙌🏼
I looked through all the code and you can find some comments inline.
As for the suggestions, you should be able to directly go to the "Files Changed" tab on GitHub and go through the comments, add the suggestions to a batch and commit them in one go.
Besides the inline comments, I was wondering whether we could fairly easily extend the supported gates here by using well known gate decompositions of these gates. Gates that come to mind are SWAP, RXX, RZZ, RYY, RZX, DCX, MCRX, MCP.
None of these should add too much complexity here. What do you think?
| TEST_F(ZXFunctionalityTest, CCZ) { | ||
| const std::string testfile = "OPENQASM 2.0;" | ||
| "include \"qelib1.inc\";" | ||
| "qreg q[3];" | ||
| "ccz q[0],q[1],q[2];\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this test use an OpenQASM file, while all others directly use the QuantumComputation API? I think I'd prefer to also have this test written as
qc.mcz(...)
| } | ||
|
|
||
| TEST_F(ZXFunctionalityTest, MultiControlX) { | ||
| using namespace qc::literals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| using namespace qc::literals; |
This isn't used here as far as I can tell.
|
|
||
| TEST_F(ZXFunctionalityTest, UnsupportedMultiControl) { | ||
| TEST_F(ZXFunctionalityTest, CRZ) { | ||
| using namespace qc::literals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| using namespace qc::literals; |
This isn't used here as far as I can tell.
| } | ||
|
|
||
| TEST_F(ZXFunctionalityTest, MultiCZ) { | ||
| using namespace qc::literals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| using namespace qc::literals; |
This isn't used here as far as I can tell.
| auto d = FunctionalityConstruction::buildFunctionality(&qc); | ||
|
|
||
| auto dPrime = FunctionalityConstruction::buildFunctionality(&qcPrime); | ||
|
|
||
| d.concat(dPrime.invert()); | ||
|
|
||
| fullReduce(d); | ||
|
|
||
| EXPECT_TRUE(FunctionalityConstruction::transformableToZX(&qc)); | ||
| EXPECT_TRUE(d.isIdentity()); | ||
| EXPECT_TRUE(d.globalPhaseIsZero()); | ||
| EXPECT_TRUE(d.connected(d.getInput(0), d.getOutput(0))); | ||
| EXPECT_TRUE(d.connected(d.getInput(1), d.getOutput(1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how often this sequence of calls is repeated throughout the tests here, I think it would be worth to extract this into a separate function in this file to reduce the amount of duplication.
| addCnot(diag, q1, q0, qubits); | ||
| addZSpider(diag, q0, qubits, -phase / 2); | ||
| addZSpider(diag, q1, qubits, phase / 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| addCnot(diag, q1, q0, qubits); | |
| addZSpider(diag, q0, qubits, -phase / 2); | |
| addZSpider(diag, q1, qubits, phase / 2); | |
| addZSpider(diag, q1, qubits, phase / 2); | |
| addCnot(diag, q1, q0, qubits); | |
| addZSpider(diag, q0, qubits, -phase / 2); |
These commute, so it does not really matter too much, but for consistency with the mcrz patter, I'd prefer this order.
| if (!anc.has_value()) { | ||
| throw ZXException("No ancilla qubit available for MCX decomposition"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unreachable code, right? There are more than nctrl+1 qubits available, so there has to be one that is not in the list of controls+target. So anc does not need to be an optional.
I also think that you could be using https://en.cppreference.com/w/cpp/algorithm/set_difference.html here.
| addCrz(diag, PiExpression(PiRational(1, 2)), nextControl, target, qubits); | ||
| addMcx(diag, controls, target, qubits); | ||
| addCrz(diag, PiExpression(-PiRational(1, 2)), nextControl, target, qubits); | ||
| addMcx(diag, controls, target, qubits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be easier to either:
- have
mczimplement the logic asH -- mcx -- H, or - have
mcxbe implemented asH -- mcz -- H
Does one of these yield a shorter decomposition?
| std::vector<Qubit> controls; | ||
| for (const auto& ctrl : op->getControls()) { | ||
| controls.push_back(static_cast<Qubit>(p.at(ctrl.qubit))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be inlined as {ctrl0, ctrl1} in the addMcrz call below.
| std::vector<Qubit> controls; | ||
| for (const auto& ctrl : op->getControls()) { | ||
| controls.push_back(static_cast<Qubit>(p.at(ctrl.qubit))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always good to use reserve to preallocate the right size for the container. And to use emplace_back over push_back.
Description
Add CRZ, MCX, MCZ, MCRZ functionality to ZX package.
Add convenience addCCZ function as it avoids one additional Hardamard edge (optimizes it away).
Fixes #1357
Checklist: