🎨 [NA] Add Dummy Decomposer to Zoned NA Compiler #902
Conversation
ystade
added
c++
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Decomposer abstraction and decomposing stage: the scheduler now emits non‑owning single‑qubit layers, Compiler invokes a Decomposer to produce owning single‑qubit layers (timed and recorded), and the code generator receives owning layers; types, scheduler API, decomposer (base + NoOp), codegen, tests, and changelog were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Compiler as Compiler
participant Scheduler as Scheduler
participant Decomposer as Decomposer
participant CodeGenerator as CodeGenerator
Client->>Compiler: compile(qc)
Compiler->>Scheduler: schedule(qc)
Scheduler-->>Compiler: (vector<SingleQubitGateRefLayer>, vector<TwoQubitGateLayer>)
Compiler->>Decomposer: decompose(singleQubitGateRefLayers)
Decomposer-->>Compiler: vector<SingleQubitGateLayer>
Compiler->>CodeGenerator: generate(decomposedSingleQubitGateLayers, twoQubitGateLayers, ...)
CodeGenerator-->>Compiler: generated artifacts
Compiler-->>Client: compiled result + statistics (decomposingTime)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-05T17:45:37.602ZApplied to files:
📚 Learning: 2025-10-14T14:37:38.047ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/Compiler.hpp (1)
84-99:decomposingTimeis missing from JSON serialization.The
decomposingTimefield is added toStatistics(line 86) but is not included in theNLOHMANN_DEFINE_TYPE_INTRUSIVE_ONLY_SERIALIZEmacro (lines 93-98). This means the decomposing time won't appear in serialized statistics output.Proposed fix
NLOHMANN_DEFINE_TYPE_INTRUSIVE_ONLY_SERIALIZE(Statistics, schedulingTime, + decomposingTime, reuseAnalysisTime, layoutSynthesizerStatistics, layoutSynthesisTime, codeGenerationTime, totalTime);
🤖 Fix all issues with AI agents
In @include/na/zoned/decomposer/DecomposerBase.hpp:
- Around line 18-21: Fix the documentation comment in the DecomposerBase class
by removing the extra space in the opening sentence: change "The Abstract Base
Class" to "The Abstract Base Class" in the comment that documents DecomposerBase
(class/struct name: DecomposerBase) so the docstring contains a single space
after "The".
In @include/na/zoned/decomposer/NoOpDecomposer.hpp:
- Around line 21-24: The class comment for NoOpDecomposer is incorrectly copied
from a scheduler; update the documentation for class NoOpDecomposer to describe
its role as a decomposer (not an "as-soon-as-possible scheduling strategy") and
state what the NoOpDecomposer does (e.g., performs no decomposition / returns
input unchanged or passes through nodes). Edit the block comment above the class
definition in NoOpDecomposer.hpp to accurately describe the class behavior and
purpose and remove any references to ASAPScheduler or scheduling.
In @include/na/zoned/Types.hpp:
- Around line 22-26: The two typedef comments are identical but describe
different ownership semantics; update the doc comments for
SingleQubitGateRefLayer and SingleQubitGateLayer to clearly state that
SingleQubitGateRefLayer is a non-owning vector of references to existing
qc::Operation objects (used when the decomposer should not take ownership),
whereas SingleQubitGateLayer is an owning vector of std::unique_ptr<const
qc::Operation> (used when the layer takes ownership and manages lifetime).
Mention typical use-cases in the decomposer pipeline for each to make the
distinction explicit.
In @test/na/zoned/test_code_generator.cpp:
- Around line 149-156: The tests create local gate objects as const (e.g., const
auto rz) and then call std::make_unique with std::move(const_obj) which is
misleading because std::move on a const object performs a copy; fix by either
making the local variable non-const (change const auto rz to auto rz) to enable
real move semantics, or remove the unnecessary std::move call and pass rz by
copy into std::make_unique; apply the same change to the other affected tests
(PGate, ZGate, etc.) and to the creation sites in SingleQubitGateLayer where
std::make_unique is called.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
include/na/zoned/Compiler.hppinclude/na/zoned/Types.hppinclude/na/zoned/decomposer/DecomposerBase.hppinclude/na/zoned/decomposer/NoOpDecomposer.hppinclude/na/zoned/scheduler/ASAPScheduler.hppinclude/na/zoned/scheduler/SchedulerBase.hppsrc/na/zoned/code_generator/CodeGenerator.cppsrc/na/zoned/decomposer/NoOpDecomposer.cppsrc/na/zoned/scheduler/ASAPScheduler.cpptest/na/zoned/test_code_generator.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
include/na/zoned/scheduler/SchedulerBase.hppinclude/na/zoned/scheduler/ASAPScheduler.hppsrc/na/zoned/scheduler/ASAPScheduler.cppinclude/na/zoned/Compiler.hpp
📚 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:
include/na/zoned/scheduler/SchedulerBase.hppsrc/na/zoned/decomposer/NoOpDecomposer.cppinclude/na/zoned/Types.hppsrc/na/zoned/code_generator/CodeGenerator.cppsrc/na/zoned/scheduler/ASAPScheduler.cpptest/na/zoned/test_code_generator.cppinclude/na/zoned/Compiler.hpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
include/na/zoned/Types.hpp
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
include/na/zoned/Types.hpp
📚 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:
include/na/zoned/Types.hppsrc/na/zoned/code_generator/CodeGenerator.cpptest/na/zoned/test_code_generator.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
src/na/zoned/code_generator/CodeGenerator.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/na/zoned/code_generator/CodeGenerator.cpp
📚 Learning: 2026-01-07T12:29:16.380Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1070-1085
Timestamp: 2026-01-07T12:29:16.380Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), the ConvertQCOFuncFuncOp pattern assumes that when a func.func operation is matched for conversion, all of its arguments are qco.qubit types (never mixed qubit/classical). The pattern unconditionally converts all arguments to qc::QubitType based on this assumption.
Applied to files:
src/na/zoned/code_generator/CodeGenerator.cpp
📚 Learning: 2025-12-17T17:44:31.349Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:31.349Z
Learning: In the QCO dialect (mlir/include/mlir/Dialect/QCO/IR/QCOOps.td), GPhaseOp intentionally uses `MemoryEffects<[MemWrite]>` instead of `Pure` to prevent the remove-dead-values pass from eliminating it. Since GPhaseOp is a zero-target operation with no result values, it would otherwise be removed by DCE, even though it has a meaningful effect on the global quantum state.
Applied to files:
src/na/zoned/code_generator/CodeGenerator.cpp
📚 Learning: 2026-01-07T12:29:02.062Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1040-1050
Timestamp: 2026-01-07T12:29:02.062Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), ConvertQCOFuncCallOp assumes that if a func::CallOp has qubit results, then all arguments and results are qubits (no mixed classical/quantum types). The conversion is scoped to handle all-qubit function calls only.
Applied to files:
src/na/zoned/code_generator/CodeGenerator.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/na/zoned/code_generator/CodeGenerator.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/na/zoned/test_code_generator.cpp
🧬 Code graph analysis (5)
src/na/zoned/decomposer/NoOpDecomposer.cpp (2)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(51-53)
include/na/zoned/decomposer/DecomposerBase.hpp (2)
include/na/zoned/Compiler.hpp (2)
nodiscard(140-259)nodiscard(261-263)include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(51-53)
src/na/zoned/scheduler/ASAPScheduler.cpp (5)
include/na/zoned/scheduler/ASAPScheduler.hpp (1)
qc(69-71)include/na/zoned/scheduler/SchedulerBase.hpp (1)
qc(34-36)include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(51-53)include/na/zoned/code_generator/CodeGenerator.hpp (1)
singleQubitGateLayers(69-71)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)
include/na/zoned/Compiler.hpp (3)
src/na/zoned/scheduler/ASAPScheduler.cpp (2)
schedule(55-130)schedule(55-57)include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(51-53)
🪛 Cppcheck (2.19.0)
src/na/zoned/decomposer/NoOpDecomposer.cpp
[information] 13-13: Include file
(missingInclude)
[information] 13-13: Include file
(missingInclude)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 13-13: Include file
(missingInclude)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 13-13: Include file
(missingInclude)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[style] 34-34: The function 'to_json' is never used.
(unusedFunction)
[style] 20-20: The function 'decompose' is never used.
(unusedFunction)
⏰ 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). (8)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (13)
src/na/zoned/decomposer/NoOpDecomposer.cpp (1)
20-34: LGTM!The implementation correctly converts reference-based layers to owning pointer-based layers by cloning each operation. The use of
reserve()for pre-allocation andstd::move()for the result layers demonstrates good performance practices.src/na/zoned/code_generator/CodeGenerator.cpp (1)
56-164: LGTM!The refactoring consistently replaces
.get()calls with the more idiomatic->operator forunique_ptraccess throughoutappendSingleQubitGates. Line 69 correctly uses*opfor dereferencing when a reference is needed fordynamic_cast.include/na/zoned/scheduler/ASAPScheduler.hpp (1)
69-71: LGTM!The return type change to
SingleQubitGateRefLayeris appropriate for a scheduler that organizes existing operations rather than creating new ones. This establishes a clear separation of concerns: the scheduler returns non-owning references, and the decomposer consumes those references to produce owned copies.include/na/zoned/scheduler/SchedulerBase.hpp (1)
34-36: LGTM!The updated return type correctly uses
SingleQubitGateRefLayerfor the first element, enabling the scheduler to return references that the decomposer will later convert to owned pointers. The interface is clean and the documentation accurately reflects the change.include/na/zoned/decomposer/DecomposerBase.hpp (1)
22-34: LGTM!The abstract interface is well-designed with clear ownership semantics: input takes references (
SingleQubitGateRefLayer), output returns owned pointers (SingleQubitGateLayer). The virtual destructor and[[nodiscard]]attribute are appropriately applied.src/na/zoned/scheduler/ASAPScheduler.cpp (2)
55-63: LGTM!The return type and local variable correctly use
SingleQubitGateRefLayer. The reference semantics are appropriate since the referenced operations inqcremain valid throughout the compilation scope.
87-92: Reference semantics correctly applied.The
emplace_backcalls properly createreference_wrapperentries:*opdereferences the unique_ptr to get the operation reference, andstdOpis already a reference from thedynamic_cast. Both correctly populate theSingleQubitGateRefLayer.test/na/zoned/test_code_generator.cpp (2)
61-72: LGTM!The Empty test correctly exercises the updated
generateinterface with an emptystd::vector<SingleQubitGateLayer>{}. The expected output is unchanged, confirming the interface change is backward-compatible for this case.
492-554: LGTM!The TwoQubitGate test correctly uses
std::vector<SingleQubitGateLayer> layers(2)to create two empty single-qubit gate layers, properly exercising the two-qubit gate code path with the new interface.include/na/zoned/Compiler.hpp (4)
47-53: LGTM!The new
Decomposertemplate parameter is cleanly integrated following the existing CRTP pattern. The inheritance chain properly includes the decomposer, enabling theSELF.decompose()call in the compilation pipeline.
205-214: LGTM!The decomposing step is correctly integrated into the compilation pipeline between scheduling and reuse analysis. Timing metrics follow the established pattern, and the ownership handoff from reference layers to owned layers is handled properly.
277-287: LGTM!
RoutingAgnosticCompilercorrectly instantiates theCompilertemplate withNoOpDecomposeras the decomposer type, maintaining consistency with the new template parameter design.
299-308: LGTM!
RoutingAwareCompilercorrectly usesNoOpDecomposer, keeping both compiler variants consistent with the new decomposer abstraction.
|
@coderabbitai please update the PR's description. Thanks! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ystade, I'll update the PR description with a comprehensive summary of the changes. 🏁 Script executed: Length of output: 55 @ystade, I've successfully updated the PR description! The description now includes:
You can view the updated description at #902. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/Compiler.hpp (1)
93-98:decomposingTimeis missing from the Statistics JSON serialization.The
decomposingTimemember was added to theStatisticsstruct (line 86), but it's not included in theNLOHMANN_DEFINE_TYPE_INTRUSIVE_ONLY_SERIALIZEmacro, so it won't be serialized to JSON. This appears to be an oversight.Suggested fix
- NLOHMANN_DEFINE_TYPE_INTRUSIVE_ONLY_SERIALIZE(Statistics, schedulingTime, + NLOHMANN_DEFINE_TYPE_INTRUSIVE_ONLY_SERIALIZE(Statistics, schedulingTime, decomposingTime, reuseAnalysisTime, layoutSynthesizerStatistics, layoutSynthesisTime, codeGenerationTime, totalTime);
🤖 Fix all issues with AI agents
In @include/na/zoned/decomposer/NoOpDecomposer.hpp:
- Around line 40-41: The inline friend function declaration 'from_json' in
NoOpDecomposer.hpp ends with a redundant trailing semicolon after its function
body; remove the semicolon following the closing brace of the friend function
definition (the friend void from_json(const BasicJsonType& /* unused */, Config&
/* unused */) { }) so the function body is terminated correctly without an extra
semicolon.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
include/na/zoned/Compiler.hppinclude/na/zoned/decomposer/NoOpDecomposer.hpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
🧬 Code graph analysis (2)
include/na/zoned/Compiler.hpp (3)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(50-52)include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)include/na/zoned/code_generator/CodeGenerator.hpp (1)
singleQubitGateLayers(69-71)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)
🔇 Additional comments (3)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
24-53: LGTM!The
NoOpDecomposerclass correctly implements theDecomposerBaseinterface with appropriate constructor signature matching other compiler components, and thedecomposemethod signature aligns with the base class definition.include/na/zoned/Compiler.hpp (2)
205-214: LGTM!The decomposition step follows the established pattern of the other compiler phases with proper timing measurement and logging.
277-287: LGTM!The compiler specializations (
RoutingAgnosticCompilerandRoutingAwareCompiler) are correctly updated to includeNoOpDecomposeras the Decomposer template parameter.Also applies to: 299-308
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yannick Stade <[email protected]>
|
@coderabbitai Can you please keep the checklist from the template description at the bottom of the PR description? |
|
@burgholzer |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/Types.hpp (1)
16-19: Missing<memory>header forstd::unique_ptr.
SingleQubitGateLayerusesstd::unique_ptrbut<memory>is not included. This will cause a compilation error on some compilers/configurations.🔧 Proposed fix
#include <array> +#include <memory> #include <unordered_map> #include <unordered_set> #include <vector>
🤖 Fix all issues with AI agents
In @include/na/zoned/decomposer/NoOpDecomposer.hpp:
- Around line 13-17: Add a direct include of the nlohmann JSON header in
NoOpDecomposer.hpp so the Config struct's use of nlohmann::detail::enable_if_t
and nlohmann::detail::is_basic_json is guaranteed to compile; specifically, add
#include <nlohmann/json.hpp> near the top of NoOpDecomposer.hpp (alongside the
existing includes) so the Config definition and any JSON-related traits resolve
without relying on transitive includes.
In @test/na/zoned/test_code_generator.cpp:
- Around line 70-84: Introduce a small test helper template (e.g.
makeSingleOpLayers) to reduce boilerplate where you construct a
SingleQubitGateLayer containing a single unique_ptr to an operation; implement
it to accept an OpType and constructor args, create a SingleQubitGateLayer,
emplace_back std::make_unique<OpType>(forward<Args>(...)), move that layer into
a vector<SingleQubitGateLayer> and return it, then replace the repeated sequence
in test_code_generator.cpp (the creation of qc::StandardOperation, wrapping in
unique_ptr, populating layer and layers) with a single call to
makeSingleOpLayers<qc::StandardOperation>(...) using the same ctor args.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
include/na/zoned/Compiler.hppinclude/na/zoned/Types.hppinclude/na/zoned/decomposer/DecomposerBase.hppinclude/na/zoned/decomposer/NoOpDecomposer.hpptest/na/zoned/test_code_generator.cpp
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-01-10T18:49:44.352Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cpp
📚 Learning: 2025-12-08T23:44:44.157Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:44.157Z
Learning: In MLIR code (mlir/ directories), avoid using const qualifiers on core MLIR data types like Value, Type, Attribute, Operation*, Block*, Region*, etc. in function parameters and signatures. MLIR's design rationale explicitly discourages const for these types. See https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Applied to files:
test/na/zoned/test_code_generator.cpp
📚 Learning: 2025-12-17T17:44:31.349Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:31.349Z
Learning: In the QCO dialect (mlir/include/mlir/Dialect/QCO/IR/QCOOps.td), GPhaseOp intentionally uses `MemoryEffects<[MemWrite]>` instead of `Pure` to prevent the remove-dead-values pass from eliminating it. Since GPhaseOp is a zero-target operation with no result values, it would otherwise be removed by DCE, even though it has a meaningful effect on the global quantum state.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cppinclude/na/zoned/Types.hpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
include/na/zoned/Types.hpp
🧬 Code graph analysis (3)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(50-52)
include/na/zoned/Compiler.hpp (3)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
singleQubitGateLayers(50-52)include/na/zoned/code_generator/CodeGenerator.hpp (1)
singleQubitGateLayers(69-71)
🔇 Additional comments (12)
include/na/zoned/Types.hpp (1)
22-27: Clean ownership semantics for layer types.The separation between
SingleQubitGateRefLayer(non-owning references from scheduling) andSingleQubitGateLayer(owning pointers from decomposition) clearly communicates ownership intent and aligns well with the compiler pipeline design.include/na/zoned/decomposer/DecomposerBase.hpp (1)
22-34: Well-designed abstract interface.The interface is minimal, focused, and correctly defines the virtual destructor. The
[[nodiscard]]attribute ondecomposeis appropriate since callers must use the returned layers.test/na/zoned/test_code_generator.cpp (3)
56-67: Empty layers test correctly adapted.The empty case properly passes an empty
std::vector<SingleQubitGateLayer>{}matching the new API signature.
470-486: Unsupported gate test correctly expects exception.The test properly verifies that unsupported operations (like
NonUnitaryOperation) throwstd::invalid_argument, ensuring robust error handling in the code generator.
487-549: Two-qubit gate integration tests properly updated.Tests correctly use
const std::vector<SingleQubitGateLayer> layers(2)to create empty single-qubit layers while testing two-qubit gate generation paths.include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
24-53: No-op decomposer implementation is correct.The class properly inherits from
DecomposerBase, provides a uniformConfigstructure for JSON serialization consistency, and the constructor signature matches the expected decomposer interface even though parameters are unused in this pass-through implementation.include/na/zoned/Compiler.hpp (6)
46-52: Template parameter and inheritance correctly extended.The
Decomposertemplate parameter is properly integrated into the class template parameter list and the protected inheritance chain, maintaining consistency with the existing CRTP pattern.
60-78: Config structure correctly extended.The
decomposerConfigfield is properly added and included in theNLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULTmacro for JSON serialization support.
84-97: Statistics structure correctly extended.The
decomposingTimefield is properly added with documentation and included in the serialization macro. The field order in the macro matches the declaration order.
203-212: Decomposition step correctly integrated into compilation pipeline.The decomposition step is properly placed between scheduling and reuse analysis. Timing measurement follows the established pattern, and the result is correctly passed to the code generator at line 241.
278-289: RoutingAgnosticCompiler correctly uses NoOpDecomposer.The specialization properly includes
NoOpDecomposeras the default decomposer, maintaining backward compatibility while enabling future decomposer substitution.
303-313: RoutingAwareCompiler correctly uses NoOpDecomposer.Consistent with
RoutingAgnosticCompiler, this specialization usesNoOpDecomposeras the default, ensuring both compiler variants have uniform decomposer behavior.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ystade, I'll fetch the PR template checklist and append it to the description. 🏁 Script executed: Length of output: 3633 🏁 Script executed: Length of output: 55 @ystade, I've successfully updated the PR description to include the checklist at the bottom! You can view it at #902. The description now contains:
(_/) 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/na/zoned/test_code_generator.cpp (1)
11-20: Missing<memory>include forstd::make_unique.The code uses
std::make_unique(e.g., line 27) but doesn't explicitly include<memory>. While this may work via transitive includes, it's not guaranteed across compilers and standard library implementations.♻️ Suggested fix
#include "ir/operations/CompoundOperation.hpp" #include "ir/operations/NonUnitaryOperation.hpp" #include "ir/operations/StandardOperation.hpp" #include "ir/operations/SymbolicOperation.hpp" #include "na/zoned/code_generator/CodeGenerator.hpp" #include <gmock/gmock-matchers.h> #include <gtest/gtest.h> +#include <memory> #include <utility> #include <vector>
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
include/na/zoned/decomposer/NoOpDecomposer.hpptest/na/zoned/test_code_generator.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-10T18:49:44.352Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cpp
🧬 Code graph analysis (1)
include/na/zoned/decomposer/NoOpDecomposer.hpp (1)
include/na/zoned/decomposer/DecomposerBase.hpp (1)
singleQubitGateLayers(31-33)
🪛 Cppcheck (2.19.0)
test/na/zoned/test_code_generator.cpp
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
⏰ 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-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (7)
test/na/zoned/test_code_generator.cpp (4)
23-31: Well-designed helper function.The
makeSingleOpLayerhelper cleanly encapsulates the boilerplate for creating test inputs, using perfect forwarding and move semantics correctly.
65-76: Test correctly validates empty layer handling.The empty test case properly validates that
CodeGenerator::generatehandles an emptySingleQubitGateLayervector, confirming the new API works correctly at the boundary.
104-123: Compound operation test construction is appropriate.The manual construction for compound operations is necessary since
makeSingleOpLayerdoesn't handle the compound case. Consider adding a comment explaining why this test doesn't use the helper, or adding an overload/variant of the helper for compound operations if more tests are added.
397-414: Two-qubit routing test uses correct empty layer pattern.Using
std::vector<SingleQubitGateLayer> layers(2)to create empty layers for routing-focused tests is appropriate since these tests validate two-qubit gate movements rather than single-qubit gate decomposition.include/na/zoned/decomposer/NoOpDecomposer.hpp (3)
1-18: Header structure and includes look correct.The includes are appropriate:
Architecture.hpp,Types.hpp, andDecomposerBase.hppprovide the necessary types, andnlohmann/json.hppsupports the Config serialization.
29-43: Empty Config with JSON serialization is appropriate.The empty
Configstruct with no-op JSON serialization correctly follows the pattern for decomposers that don't require configuration, while maintaining API consistency with other decomposers.
48-53: Constructor and decompose declaration match the expected interface.The constructor maintains consistency with other decomposers (taking
Architecture&andConfig&), and thedecomposesignature correctly matches the base class. Theoverridekeyword ensures compile-time validation. The implementation correctly converts non-owning reference wrappers (SingleQubitGateRefLayer) to owning unique pointers (SingleQubitGateLayer) by callingclone()on each operation, with efficient use ofreserve()and move semantics.
|
@burgholzer as you can see in fe75dbd, the CI was green, even though @coderabbitai noticed two missing headers posted as "comments outside of diff". Is this supposed to be or should the CI actually fail? |
|
Tip For best results, initiate chat on the files or code changes. @ystade, great question! CodeRabbit is designed to complement, not replace, human review, and by default, its review comments are advisory and informational — they don't automatically fail CI checks. Here's why the CI was green despite CodeRabbit flagging issues:
So to answer your question: This is the expected behavior. CodeRabbit provides guidance, but CI passing is independent of whether CodeRabbit flags issues. If blocking is desired, it needs explicit configuration in your repository settings or CodeRabbit configuration file. Would you like me to help configure CodeRabbit to have more blocking behavior, or is the current advisory mode working well for your workflow? 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/na/zoned/test_code_generator.cpp:
- Around line 24-32: In makeSingleOpLayer replace the stack-constructed OpType
and subsequent move with direct heap construction: create the unique_ptr by
calling std::make_unique<OpType>(std::forward<Args>(args)...) and emplace_back
that into the SingleQubitGateLayer so you avoid constructing a local OpType
named op and moving it; keep the rest (wrapping the layer into layers and
returning) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
include/na/zoned/Types.hpptest/na/zoned/test_code_generator.cpp
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-01-10T18:49:44.352Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
test/na/zoned/test_code_generator.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/na/zoned/test_code_generator.cppinclude/na/zoned/Types.hpp
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
include/na/zoned/Types.hpp
🪛 Cppcheck (2.19.0)
test/na/zoned/test_code_generator.cpp
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
⏰ 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 (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (4)
include/na/zoned/Types.hpp (1)
23-28: Clear ownership semantics with well-documented type aliases.The separation between non-owning
SingleQubitGateRefLayer(for scheduler output) and owningSingleQubitGateLayer(for post-decomposition) is a clean design that makes ownership explicit at the type level. Theconstqualifier on the ownedqc::Operationappropriately signals immutability.test/na/zoned/test_code_generator.cpp (3)
66-77: Test correctly validates empty layer handling.The test properly exercises the code generator with an empty
SingleQubitGateLayervector and verifies the expected atom output.
105-124: Manual layer construction is appropriate for compound operations.The
CompoundOperationrequires building up its inner operations before wrapping in aunique_ptr, so the manual construction here is necessary and correctly follows the new ownership semantics.
398-415: Two-qubit gate tests correctly use empty single-qubit layers.Using
std::vector<SingleQubitGateLayer>(2)to create two empty layers is correct for tests focused on two-qubit gate operations and routing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 14-16: Summary: The changelog entry uses "(dummy)" which may be
unclear; update the phrasing. Edit CHANGELOG.md and locate the entry "✨ Add a
(dummy) decomposer component to the zoned neutral atom compiler" and replace
"(dummy)" with a clearer term (examples: "no-op", "pass-through", or remove the
qualifier entirely) so the line reads e.g. "✨ Add a no-op decomposer component
to the zoned neutral atom compiler" while keeping the PR reference and author
attribution unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
📚 Learning: 2025-10-14T14:37:38.047Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Applied to files:
CHANGELOG.md
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 16: Add the missing PR link definition for reference [`#902`] in
CHANGELOG.md by inserting a new PR link entry in the PR links section using the
same link-definition pattern as other PR entries (i.e., the "[`#NNN`]: <PR URL>"
style) and place it so the PR links remain in reverse numerical order; ensure
the URL points to the pull request number 902 in the repository and matches the
formatting of existing entries.
- Around line 23-26: Remove the duplicate release header by keeping the version
header that includes the date ("## [3.5.0] - 2025-12-16") and deleting the
subsequent bare "## [3.5.0]" line so only the dated header remains; update any
surrounding spacing to ensure a single clean header for version 3.5.0.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
📚 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:
CHANGELOG.md
📚 Learning: 2025-10-14T14:37:38.047Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Applied to files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[typographical] ~23-~23: To join two clauses or introduce examples, consider using an em dash.
Context: ...([#911]) ([@denialhaag]) ## [3.5.0] - 2025-12-16 ## [3.5.0] _If you are upgr...
(DASH_RULE)
⏰ 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 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Overview
This PR adds a decomposer abstraction to the Zoned Neutral Atom (NA) Compiler, enabling transformation of single-qubit gate layers into the native gate set. The implementation introduces a new
Decomposertemplate parameter to the compiler pipeline and provides aNoOpDecomposeras the default implementation.Key Changes
Architecture Changes
Decomposeras a template parameter to theCompilerclass, extending the compilation pipelineSingleQubitGateRefLayerfor non-owning references to operationsSingleQubitGateLayerto useunique_ptrfor owning semanticsdecomposerConfigfield toCompiler::Configwith JSON serialization supportdecomposingTimetoCompiler::Statisticsfor performance trackingNew Components
DecomposerBase: Abstract base class defining the decomposer interface with a pure virtualdecomposemethodNoOpDecomposer: Concrete implementation that performs a pass-through decomposition by cloning input operationsScheduler Updates
SchedulerBaseandASAPSchedulerto returnSingleQubitGateRefLayerinstead ofSingleQubitGateLayerCode Generator Updates
CodeGeneratorto work withunique_ptr-basedSingleQubitGateLayer.get()calls with->operator for cleaner smart pointer accessCompiler Integration
RoutingAgnosticCompilerandRoutingAwareCompilernow instantiate withNoOpDecomposerby defaultImplementation Details
The decomposer sits between scheduling and layout synthesis in the compilation pipeline:
SingleQubitGateRefLayer(non-owning references)SingleQubitGateLayer(owned operations)The
NoOpDecomposerprovides a baseline implementation that simply clones all operations, maintaining backward compatibility while establishing the infrastructure for future decomposer implementations.Testing
SingleQubitGateLayerwithunique_ptr-owned operationsChecklist: