🎨 [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. WalkthroughThe PR adds a Decomposer abstraction to the zoned compiler: new Decomposer template parameter, Config and Statistics fields, a decomposition step that converts reference-based single-qubit layers into owning unique_ptr-based layers, a DecomposerBase interface and NoOpDecomposer implementation, and related type/signature changes across scheduler, code generator, and tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Compiler
participant Scheduler
participant Decomposer
participant CodeGenerator
Client->>Compiler: compile(quantumComputation)
Compiler->>Scheduler: schedule(qc)
Scheduler-->>Compiler: (vector<SingleQubitGateRefLayer>, vector<TwoQubitGateLayer>)
Compiler->>Decomposer: decompose(singleQubitGateRefLayers)
Decomposer-->>Compiler: vector<SingleQubitGateLayer> (cloned owning ops)
Compiler->>CodeGenerator: generate(singleQubitGateLayers, ...)
CodeGenerator-->>Compiler: generated code / artifacts
Compiler-->>Client: compiled result / stats (includes decomposingTime)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
![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.
| /** | ||
| * The Abstract Base Class for the Decomposer of the MQT's Zoned Neutral Atom | ||
| * Compiler. | ||
| */ |
There was a problem hiding this comment.
Minor typo in documentation comment.
There's an extra space in "The Abstract Base Class" (double space after "The").
Proposed fix
-/**
- * The Abstract Base Class for the Decomposer of the MQT's Zoned Neutral Atom
- * Compiler.
- */
+/**
+ * The Abstract Base Class for the Decomposer of the MQT's Zoned Neutral Atom
+ * Compiler.
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * The Abstract Base Class for the Decomposer of the MQT's Zoned Neutral Atom | |
| * Compiler. | |
| */ | |
| /** | |
| * The Abstract Base Class for the Decomposer of the MQT's Zoned Neutral Atom | |
| * Compiler. | |
| */ |
🤖 Prompt for AI Agents
In @include/na/zoned/decomposer/DecomposerBase.hpp around lines 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".
| /// A list of single-qubit gates representing a single-qubit gate layer. | ||
| using SingleQubitGateLayer = | ||
| using SingleQubitGateRefLayer = | ||
| std::vector<std::reference_wrapper<const qc::Operation>>; | ||
| /// A list of single-qubit gates representing a single-qubit gate layer. | ||
| using SingleQubitGateLayer = std::vector<std::unique_ptr<const qc::Operation>>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the distinction between the two layer types in documentation.
Lines 22 and 25 have identical comments, but SingleQubitGateRefLayer (non-owning references) and SingleQubitGateLayer (owning unique pointers) serve different purposes in the decomposer pipeline. Consider distinguishing them in the documentation.
📝 Suggested documentation improvement
-/// A list of single-qubit gates representing a single-qubit gate layer.
+/// A non-owning reference layer of single-qubit gates (used for scheduling output).
using SingleQubitGateRefLayer =
std::vector<std::reference_wrapper<const qc::Operation>>;
-/// A list of single-qubit gates representing a single-qubit gate layer.
+/// An owning layer of single-qubit gates (used after decomposition).
using SingleQubitGateLayer = std::vector<std::unique_ptr<const qc::Operation>>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// A list of single-qubit gates representing a single-qubit gate layer. | |
| using SingleQubitGateLayer = | |
| using SingleQubitGateRefLayer = | |
| std::vector<std::reference_wrapper<const qc::Operation>>; | |
| /// A list of single-qubit gates representing a single-qubit gate layer. | |
| using SingleQubitGateLayer = std::vector<std::unique_ptr<const qc::Operation>>; | |
| /// A non-owning reference layer of single-qubit gates (used for scheduling output). | |
| using SingleQubitGateRefLayer = | |
| std::vector<std::reference_wrapper<const qc::Operation>>; | |
| /// An owning layer of single-qubit gates (used after decomposition). | |
| using SingleQubitGateLayer = std::vector<std::unique_ptr<const qc::Operation>>; |
🤖 Prompt for AI Agents
In @include/na/zoned/Types.hpp around lines 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.
| TEST_F(CodeGeneratorGenerateTest, RZGate) { | ||
| const auto& slm = *architecture.storageZones.front(); | ||
| const auto rz = qc::StandardOperation(0, qc::RZ, {0.1}); | ||
| SingleQubitGateLayer layer; | ||
| layer.emplace_back(std::make_unique<qc::StandardOperation>(std::move(rz))); | ||
| std::vector<SingleQubitGateLayer> layers; | ||
| layers.emplace_back(std::move(layer)); | ||
| EXPECT_EQ( |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Remove misleading std::move on const objects.
The pattern const auto rz = ...; std::make_unique<...>(std::move(rz)) is misleading—std::move on a const object results in a copy, not a move. While this works correctly, removing const would enable actual move semantics, or you could simply remove std::move to make the copy explicit.
This pattern appears in multiple tests (RZGate, PGate, ZGate, etc.).
Example fix for one test
TEST_F(CodeGeneratorGenerateTest, RZGate) {
const auto& slm = *architecture.storageZones.front();
- const auto rz = qc::StandardOperation(0, qc::RZ, {0.1});
+ auto rz = qc::StandardOperation(0, qc::RZ, {0.1});
SingleQubitGateLayer layer;
layer.emplace_back(std::make_unique<qc::StandardOperation>(std::move(rz)));🤖 Prompt for AI Agents
In @test/na/zoned/test_code_generator.cpp around lines 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.
|
@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
| friend void from_json(const BasicJsonType& /* unused */, | ||
| Config& /* unused */) {}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Remove trailing semicolon after function body.
The semicolon after the closing brace of from_json is unnecessary.
Suggested fix
friend void from_json(const BasicJsonType& /* unused */,
- Config& /* unused */) {};
+ Config& /* unused */) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| friend void from_json(const BasicJsonType& /* unused */, | |
| Config& /* unused */) {}; | |
| friend void from_json(const BasicJsonType& /* unused */, | |
| Config& /* unused */) {} |
🤖 Prompt for AI Agents
In @include/na/zoned/decomposer/NoOpDecomposer.hpp around lines 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.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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 operations