🎨 [NA] Add 2Q-Gate Layer to Decomposer Interface#976
Open
Conversation
ystade
added
c++
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCompiler and decomposer interfaces updated so the decomposer accepts both single‑qubit and two‑qubit gate layer inputs and returns decomposed single‑ and two‑qubit layers; compiler pipeline, reuse analysis, and layout synthesis now consume the decomposed two‑qubit layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Compiler
participant Decomposer
participant ReuseAnalyzer
Client->>Compiler: compile(zoned circuit)
Compiler->>Decomposer: decompose(singleQubitGateLayers, twoQubitGateLayers)
Decomposer-->>Compiler: (decomposedSingleQubitGateLayers, decomposedTwoQubitGateLayers)
Compiler->>ReuseAnalyzer: analyze(decomposedTwoQubitGateLayers)
ReuseAnalyzer-->>Compiler: reuse decisions
Compiler-->>Client: compiled output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/na/zoned/Compiler.hpp (1)
217-228:⚠️ Potential issue | 🟠 MajorInconsistent usage of original vs. decomposed two-qubit gate layers.
Line 217 passes
decomposedTwoQubitGateLayerstoanalyzeReuse, but line 228 passes the originaltwoQubitGateLayerstosynthesize. When a real decomposer that modifies 2Q gates is introduced (as mentioned in the PR objectives), this inconsistency will cause the reuse analysis and layout synthesis to operate on different gate sets, likely leading to incorrect compilation results.Should line 228 also use
decomposedTwoQubitGateLayers?Proposed fix
const auto& [placement, routing] = LayoutSynthesizer::synthesize( - qComp.getNqubits(), twoQubitGateLayers, reuseQubits); + qComp.getNqubits(), decomposedTwoQubitGateLayers, reuseQubits);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/na/zoned/Compiler.hpp` around lines 217 - 228, The reuse analysis is run on decomposedTwoQubitGateLayers via SELF.analyzeReuse but LayoutSynthesizer::synthesize is called with the original twoQubitGateLayers, causing inconsistent inputs; change the call to LayoutSynthesizer::synthesize to pass decomposedTwoQubitGateLayers (along with qComp.getNqubits() and reuseQubits) so layout synthesis uses the same decomposed gate set as analyzeReuse.include/na/zoned/decomposer/DecomposerBase.hpp (1)
25-35: 🧹 Nitpick | 🔵 TrivialIncomplete documentation for the updated interface.
The docstring is missing
@param twoQubitGateLayersand the@returndescription needs to reflect that a pair is now returned (including both decomposed single-qubit and two-qubit gate layers).Proposed documentation update
/** * This function defines the interface of the decomposer. * `@param` singleQubitGateLayers are the layers of single-qubit gates that are * meant to be first decomposed into the native gate set. - * `@return` the new single-qubit gate layers + * `@param` twoQubitGateLayers are the layers of two-qubit gates that may be + * transformed by the decomposer. + * `@return` a pair containing the decomposed single-qubit gate layers and + * the (potentially transformed) two-qubit gate layers. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/na/zoned/decomposer/DecomposerBase.hpp` around lines 25 - 35, The method documentation for DecomposerBase::decompose is out of date: add a `@param` entry for twoQubitGateLayers and update `@return` to describe that the function returns a std::pair containing the decomposed single-qubit gate layers and the decomposed two-qubit gate layers. Locate the decompose(...) declaration and expand the comment block to include "@param twoQubitGateLayers the layers of two-qubit gates to be considered during decomposition" and update "@return" to clearly state that it returns a pair<std::vector<SingleQubitGateLayer>, std::vector<TwoQubitGateLayer>> representing the new single-qubit and two-qubit gate layers respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@include/na/zoned/Compiler.hpp`:
- Around line 217-228: The reuse analysis is run on decomposedTwoQubitGateLayers
via SELF.analyzeReuse but LayoutSynthesizer::synthesize is called with the
original twoQubitGateLayers, causing inconsistent inputs; change the call to
LayoutSynthesizer::synthesize to pass decomposedTwoQubitGateLayers (along with
qComp.getNqubits() and reuseQubits) so layout synthesis uses the same decomposed
gate set as analyzeReuse.
In `@include/na/zoned/decomposer/DecomposerBase.hpp`:
- Around line 25-35: The method documentation for DecomposerBase::decompose is
out of date: add a `@param` entry for twoQubitGateLayers and update `@return` to
describe that the function returns a std::pair containing the decomposed
single-qubit gate layers and the decomposed two-qubit gate layers. Locate the
decompose(...) declaration and expand the comment block to include "@param
twoQubitGateLayers the layers of two-qubit gates to be considered during
decomposition" and update "@return" to clearly state that it returns a
pair<std::vector<SingleQubitGateLayer>, std::vector<TwoQubitGateLayer>>
representing the new single-qubit and two-qubit gate layers respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7d49d0c-42a4-47b4-9b6e-fe5fffd4b555
📒 Files selected for processing (4)
include/na/zoned/Compiler.hppinclude/na/zoned/decomposer/DecomposerBase.hppinclude/na/zoned/decomposer/NoOpDecomposer.hppsrc/na/zoned/decomposer/NoOpDecomposer.cpp
burgholzer
approved these changes
Apr 3, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 14-17: Add an UPGRADING.md entry under the [Unreleased] section
documenting the breaking change to the DecomposerBase interface: the decompose()
method now accepts an additional twoQubitGateLayers parameter and its return
type changed to std::pair; instruct maintainers to update any custom
DecomposerBase subclasses by adding the new parameter to their decompose()
implementations, returning the new std::pair form (describe both elements of the
pair), and provide a short example/migration snippet showing the old
signature->new signature mapping and how to construct the std::pair result for
compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/na/zoned/decomposer/DecomposerBase.hpp`:
- Around line 27-40: The decompose() API currently returns an unnamed std::pair
which makes the contract (which element is the single-qubit layers vs two-qubit
layers) implicit and error-prone; define a named return type (e.g., struct
DecompositionResult { std::vector<SingleQubitGateLayer> singleQubitLayers;
std::vector<TwoQubitGateLayer> twoQubitLayers; }) and update
DecomposerBase::decompose signature to return DecompositionResult (and update
the comment to explicitly state which member maps to generate() and which maps
to analyzeReuse()/synthesize()), then adjust any callers to use the named fields
instead of std::pair::first/second.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62fb42dd-dc97-4917-a328-82e6fa44e6e8
📒 Files selected for processing (2)
include/na/zoned/Compiler.hppinclude/na/zoned/decomposer/DecomposerBase.hpp
Comment on lines
+27
to
+40
| * | ||
| * The decomposer may change the layering produced by the scheduler and, | ||
| * hence, it receives the single-qubit and two-qubit gate layers. | ||
| * @param singleQubitGateLayers are the layers of single-qubit gates that are | ||
| * meant to be first decomposed into the native gate set. | ||
| * @return the new single-qubit gate layers | ||
| * @param twoQubitGateLayers are the layers of two-qubit gates that the | ||
| * decomposer may change. | ||
| * @return a new pair of single-qubit and two-qubit gate layers | ||
| */ | ||
| [[nodiscard]] virtual auto decompose( | ||
| const std::vector<SingleQubitGateRefLayer>& singleQubitGateLayers) const | ||
| -> std::vector<SingleQubitGateLayer> = 0; | ||
| [[nodiscard]] virtual auto | ||
| decompose(const std::vector<SingleQubitGateRefLayer>& singleQubitGateLayers, | ||
| const std::vector<TwoQubitGateLayer>& twoQubitGateLayers) const | ||
| -> std::pair<std::vector<SingleQubitGateLayer>, | ||
| std::vector<TwoQubitGateLayer>> = 0; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the new decomposition result contract explicit.
The compiler now feeds the returned 2Q layers into analyzeReuse()/synthesize() and the returned 1Q layers into generate(). A raw std::pair plus the current doc leaves the required alignment invariant implicit; documenting it here, or wrapping it in a named DecompositionResult, would make future decomposers harder to misuse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/na/zoned/decomposer/DecomposerBase.hpp` around lines 27 - 40, The
decompose() API currently returns an unnamed std::pair which makes the contract
(which element is the single-qubit layers vs two-qubit layers) implicit and
error-prone; define a named return type (e.g., struct DecompositionResult {
std::vector<SingleQubitGateLayer> singleQubitLayers;
std::vector<TwoQubitGateLayer> twoQubitLayers; }) and update
DecomposerBase::decompose signature to return DecompositionResult (and update
the comment to explicitly state which member maps to generate() and which maps
to analyzeReuse()/synthesize()), then adjust any callers to use the named fields
instead of std::pair::first/second.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We are implementing an actual decomposer, replacing the current
NoOpDecomposer. Because this new Decomposer has an optimzation routine that also affects the two-qubit gates, the decomposer must be able to modify the two-qubit gates. Additionally, one could imagine, that the decomposer might also transform two-qubit gates.Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.