✨ Add buffer size support to HybridSynthesisMapper #885
Conversation
…mapping functionality
There was a problem hiding this comment.
Pull request overview
This PR adds buffer functionality to the ZX hybrid synthesis mapper, enabling staged operation management before mapping to hardware. The changes introduce a bufferSize parameter and bufferedQc quantum computation to temporarily store operations, along with support for complete remapping and more flexible synthesis evaluation.
Key Changes
- Added buffer mechanism (
bufferedQcandbufferSize) to stage operations before mapping - Refactored
appendWithMappingto support optional complete remapping - Enhanced parametrized tests to cover different
completeRemapandalsoMapcombinations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
include/hybridmap/HybridSynthesisMapper.hpp |
Added buffer-related member variables, refactored completeRemap and appendWithMapping signatures, enhanced getSynthesizedQc to include buffered operations |
src/hybridmap/HybridSynthesisMapper.cpp |
Implemented buffer injection logic in evaluateSynthesisStep, merged append functions, improved variable scoping |
test/hybridmap/test_hybrid_synthesis_map.cpp |
Added TestParams struct for parametrized testing, expanded test cases to 12 combinations, removed obsolete tests |
bindings/hybrid_mapper/hybrid_mapper.cpp |
Updated Python bindings to expose new parameters, removed append_without_mapping binding, simplified complete_remap interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds buffering to HybridSynthesisMapper (bufferedQc, bufferSize, originalMapping), extends constructor and public API (evaluate/append/completeRemap/schedule) to support buffer-aware remapping, updates Python bindings and tests to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HSM as HybridSynthesisMapper
participant Buffer as BufferedQc
participant Mapper as MappingEngine
Client->>HSM: evaluateSynthesisSteps(synthesisSteps, completeRemap, alsoMap)
loop per synthesis step
HSM->>HSM: evaluateSynthesisStep(stepQc, completeRemap)
alt buffer non-empty
HSM->>Buffer: read bufferedQc (reverse_view)
Buffer-->>HSM: buffered ops (reversed)
HSM->>HSM: merge buffered ops into toBeMappedQc
end
HSM->>HSM: promote buffered entries to synthesizedQc when threshold reached
alt completeRemap = true
HSM->>Mapper: completeRemap(includeBuffer?)
Mapper-->>HSM: updated global mapping
else
HSM->>Mapper: map toBeMappedQc incrementally
Mapper-->>HSM: mapping applied for toBeMappedQc
end
HSM->>HSM: append mapped ops to synthesizedQc
end
HSM-->>Client: return evaluation results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/hybridmap/test_hybrid_synthesis_map.cpp (1)
66-80: Test parameterscompleteRemapandalsoMapare defined but never used.The
TestParamsstruct definescompleteRemapandalsoMapfields, and 12 combinations are instantiated, but these values are never passed to the methods under test. Line 76 hardcodestrueinstead of usingGetParam().completeRemapandGetParam().alsoMap.Apply this diff to use the parameterized values:
- const auto best = mapper.evaluateSynthesisSteps(circuits, true); + const auto best = mapper.evaluateSynthesisSteps(circuits, GetParam().completeRemap, GetParam().alsoMap);src/hybridmap/HybridSynthesisMapper.cpp (2)
31-67: MissingcompleteRemapparameter inevaluateSynthesisStepcall.Line 46 calls
evaluateSynthesisStep(qc)without passing thecompleteRemapparameter. While the parameter has a default value offalse, the intent appears to be passing thecompleteRemapflag received byevaluateSynthesisSteps.Apply this diff:
- const auto fidelity = evaluateSynthesisStep(qc); + const auto fidelity = evaluateSynthesisStep(qc, completeRemap);
69-89: Inputqcis mutated in place, causing unintended side effects.The method modifies the input
qcreference by callingqc.reverse()and inserting buffered operations (lines 77-81). Sinceqcis passed by reference from the loop inevaluateSynthesisSteps, each candidate circuit insynthesisStepswill be permanently modified after evaluation. This corrupts the caller's data.Work on a copy instead of the original:
qc::fp HybridSynthesisMapper::evaluateSynthesisStep(qc::QuantumComputation& qc, const bool completeRemap) const { NeutralAtomMapper tempMapper(arch, parameters); if (!completeRemap) { tempMapper.copyStateFrom(*this); } - qc.reverse(); + // Work on a copy to avoid mutating the input + auto qcCopy = qc; + qcCopy.reverse(); // insert buffered operations for (const auto& opPointer : std::ranges::reverse_view(bufferedQc)) { - qc.emplace_back(opPointer->clone()); + qcCopy.emplace_back(opPointer->clone()); } - // Make a copy of qc to avoid modifying the original - auto qcCopy = qc; auto mappedQc = tempMapper.map(qcCopy, mapping); tempMapper.convertToAod(); const auto results = tempMapper.schedule(); return results.totalFidelities; }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
bindings/hybrid_mapper/hybrid_mapper.cpp(3 hunks)include/hybridmap/HybridSynthesisMapper.hpp(6 hunks)src/hybridmap/HybridSynthesisMapper.cpp(4 hunks)test/hybridmap/test_hybrid_synthesis_map.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cppsrc/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cppsrc/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridSynthesisMapper.hppbindings/hybrid_mapper/hybrid_mapper.cpp
📚 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:
src/hybridmap/HybridSynthesisMapper.cpp
📚 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/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-12-15T01:54:22.129Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/dd/register_matrix_dds.cpp:64-109
Timestamp: 2025-12-15T01:54:22.129Z
Learning: In the munich-quantum-toolkit/core repository, after migrating to nanobind, docstrings for Python bindings are now added directly in the C++ binding code (using R"pb(...)pb" syntax) and stub files (.pyi) are auto-generated using the `bindings/generate-stubs.sh` script. This replaces the previous pybind11 approach where docstrings were manually maintained in stub files.
Applied to files:
bindings/hybrid_mapper/hybrid_mapper.cpp
🧬 Code graph analysis (3)
test/hybridmap/test_hybrid_synthesis_map.cpp (2)
test/hybridmap/test_hybridmap.cpp (6)
testArchitecturePath(30-30)mapper(94-94)mapper(227-227)mapper(263-263)mapper(272-272)mapper(285-285)include/hybridmap/HybridSynthesisMapper.hpp (2)
qc(59-60)qc(178-179)
src/hybridmap/HybridSynthesisMapper.cpp (1)
include/hybridmap/HybridSynthesisMapper.hpp (3)
synthesisSteps(169-171)qc(59-60)qc(178-179)
include/hybridmap/HybridSynthesisMapper.hpp (4)
include/hybridmap/HybridNeutralAtomMapper.hpp (8)
qc(549-556)qc(549-550)qc(558-563)qc(558-559)qc(571-571)qc(681-681)op(134-134)op(385-386)include/hybridmap/NeutralAtomScheduler.hpp (1)
qc(136-140)include/hybridmap/Mapping.hpp (6)
Mapping(61-61)Mapping(66-70)Mapping(66-66)Mapping(81-107)op(199-204)op(199-199)src/hybridmap/HybridNeutralAtomMapper.cpp (2)
mapAppend(45-123)mapAppend(45-46)
🪛 Cppcheck (2.18.0)
test/hybridmap/test_hybrid_synthesis_map.cpp
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
src/hybridmap/HybridSynthesisMapper.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] 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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] 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] 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingInclude)
[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] 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] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 31-31: The function 'evaluateSynthesisSteps' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 71-71: The function 'toCsv' is never used.
(unusedFunction)
[style] 68-68: The function 'initialCoordinateMappingFromString' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 91-91: The function 'initialMappingFromString' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
bindings/hybrid_mapper/hybrid_mapper.cpp
[error] 278-278: Boolean value assigned to pointer.
(assignBoolToPointer)
[style] 276-276: The function 'getOffsetDistance' is never used.
(unusedFunction)
[error] 303-303: Boolean value assigned to pointer.
(assignBoolToPointer)
[error] 303-303: Boolean value assigned to pointer.
(assignBoolToPointer)
[style] 300-300: The function 'getNearbyQubits' is never used.
(unusedFunction)
[style] 301-301: The function 'append' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
🔇 Additional comments (12)
test/hybridmap/test_hybrid_synthesis_map.cpp (3)
28-32: TestParams struct is well-defined.The struct cleanly encapsulates the test parameters for architecture and the new boolean flags.
82-95: Good coverage of parameter combinations.The 12 combinations properly cover all permutations of 3 architectures × 2
completeRemapvalues × 2alsoMapvalues.
121-131: Output test correctly exercises the newcompleteRemapparameter.The call
mapper.appendWithMapping(qc, true)properly tests the new API signature.src/hybridmap/HybridSynthesisMapper.cpp (2)
91-104:appendWithMappingimplementation looks correct.The method properly handles both paths: cloning operations to
synthesizedQcand then either performing a complete remap or a partialmapAppendbased on the flag.
106-132: Good use of structured binding in if-init statement.The refactored adjacency matrix lookup using
if (const auto mappedJ = ...; ...)is cleaner and reduces repeated lookups.bindings/hybrid_mapper/hybrid_mapper.cpp (3)
226-234: Constructor binding correctly exposes the newbuffer_sizeparameter.The binding properly adds the
buffer_sizeparameter with a default value of 0, matching the C++ API.
270-278:append_with_mappingbinding correctly exposescomplete_remapparameter.The binding properly wraps the C++ method and exposes the new parameter with the appropriate default value.
294-303:evaluate_synthesis_stepsbinding correctly exposes new parameters.The binding properly wraps the method with
complete_remapandalso_mapparameters, matching the C++ API defaults.include/hybridmap/HybridSynthesisMapper.hpp (4)
46-48: New buffer-related members are well-organized.The addition of
bufferSize,bufferedQc, andoriginalMappingclearly supports the new buffering functionality.
71-75: Constructor correctly initializesbufferSize.The new parameter is properly added with a sensible default value.
128-138:getSynthesizedQcefficiently combines circuits.The method properly reserves capacity and clones operations from both
synthesizedQcandbufferedQc.
144-160: Export methods correctly usegetSynthesizedQc().Both
getSynthesizedQcQASM()andsaveSynthesizedQc()properly use the combined view that includes buffered operations.
…nnecessary parameter
…brid Synthesis Mapper
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 (2)
test/hybridmap/test_hybrid_synthesis_map.cpp (1)
82-85: Test instantiation provides wrong parameter type.
INSTANTIATE_TEST_SUITE_Ppasses plain strings, but the test fixture now expectsTestParamsstructs. This will fail to compile since there's no implicit conversion fromconst char*toTestParams.Apply this diff to fix the test instantiation:
INSTANTIATE_TEST_SUITE_P(HybridSynthesisMapperTestSuite, TestParametrizedHybridSynthesisMapper, - ::testing::Values("rubidium_gate", "rubidium_hybrid", - "rubidium_shuttling")); + ::testing::Values( + TestParams{"rubidium_gate", false, false}, + TestParams{"rubidium_hybrid", false, false}, + TestParams{"rubidium_shuttling", false, false}));src/hybridmap/HybridSynthesisMapper.cpp (1)
43-52:completeRemapparameter is not passed toevaluateSynthesisStep.The
completeRemapparameter is accepted byevaluateSynthesisStepsbut is not forwarded toevaluateSynthesisStepat line 47. This means all synthesis step evaluations use the default (false) regardless of the caller's intent.Apply this diff to forward the parameter:
for (auto& qc : synthesisSteps) { if (parameters.verbose) { spdlog::info("Evaluating synthesis step number {}", qcIndex); } - const auto fidelity = evaluateSynthesisStep(qc); + const auto fidelity = evaluateSynthesisStep(qc, completeRemap); candidates.emplace_back(qc, fidelity);
♻️ Duplicate comments (1)
test/hybridmap/test_hybrid_synthesis_map.cpp (1)
28-32:TestParams.completeRemapandTestParams.alsoMapare defined but never used.The
TestParamsstruct definescompleteRemapandalsoMapmembers, but theEvaluateSynthesisSteptest hardcodestrueat line 76 instead of usingGetParam().completeRemap. Either use the parameters or simplifyTestParamsto only containarchitecture.If parametrization is intended, use the struct members:
- const auto best = mapper.evaluateSynthesisSteps(circuits, true); + const auto best = mapper.evaluateSynthesisSteps(circuits, GetParam().completeRemap, GetParam().alsoMap);Also applies to: 66-80
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
bindings/hybrid_mapper/hybrid_mapper.cpp(3 hunks)include/hybridmap/HybridSynthesisMapper.hpp(6 hunks)src/hybridmap/HybridSynthesisMapper.cpp(3 hunks)test/hybridmap/test_hybrid_synthesis_map.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cpptest/hybridmap/test_hybrid_synthesis_map.cppbindings/hybrid_mapper/hybrid_mapper.cppinclude/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
src/hybridmap/HybridSynthesisMapper.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/hybridmap/HybridSynthesisMapper.cpp
📚 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:
src/hybridmap/HybridSynthesisMapper.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cpptest/hybridmap/test_hybrid_synthesis_map.cppinclude/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cppinclude/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-12-15T01:54:22.129Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/dd/register_matrix_dds.cpp:64-109
Timestamp: 2025-12-15T01:54:22.129Z
Learning: In the munich-quantum-toolkit/core repository, after migrating to nanobind, docstrings for Python bindings are now added directly in the C++ binding code (using R"pb(...)pb" syntax) and stub files (.pyi) are auto-generated using the `bindings/generate-stubs.sh` script. This replaces the previous pybind11 approach where docstrings were manually maintained in stub files.
Applied to files:
bindings/hybrid_mapper/hybrid_mapper.cpp
📚 Learning: 2025-12-10T02:20:01.189Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:0-0
Timestamp: 2025-12-10T02:20:01.189Z
Learning: In QIRProgramBuilder::measure(Value, int64_t), the intentional design prevents duplicate measurements: the early return when `registerResultMap.find(key)` succeeds avoids generating multiple `mz` calls to the same classical bit index and ensures output recording contains only one entry per index. This implements an "override" semantic where repeated measurements to the same resultIndex reuse the cached pointer without additional side effects.
Applied to files:
include/hybridmap/HybridSynthesisMapper.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/hybridmap/HybridSynthesisMapper.hpp
🧬 Code graph analysis (3)
src/hybridmap/HybridSynthesisMapper.cpp (1)
include/hybridmap/HybridSynthesisMapper.hpp (3)
synthesisSteps(171-173)qc(61-62)qc(180-181)
bindings/hybrid_mapper/hybrid_mapper.cpp (4)
python/mqt/qmap/hybrid_mapper.pyi (1)
MapperParameters(24-40)test/hybridmap/test_hybridmap.cpp (5)
mapper(94-94)mapper(227-227)mapper(263-263)mapper(272-272)mapper(285-285)include/hybridmap/HybridSynthesisMapper.hpp (2)
qc(61-62)qc(180-181)test/hybridmap/test_hybrid_synthesis_map.cpp (1)
qc(94-101)
include/hybridmap/HybridSynthesisMapper.hpp (3)
include/hybridmap/HybridNeutralAtomMapper.hpp (8)
qc(549-556)qc(549-550)qc(558-563)qc(558-559)qc(571-571)qc(681-681)op(134-134)op(385-386)include/hybridmap/NeutralAtomScheduler.hpp (1)
qc(136-140)include/hybridmap/Mapping.hpp (6)
Mapping(61-61)Mapping(66-70)Mapping(66-66)Mapping(81-107)op(199-204)op(199-199)
🪛 Cppcheck (2.18.0)
src/hybridmap/HybridSynthesisMapper.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingInclude)
[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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 32-32: The function 'evaluateSynthesisSteps' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[performance] 105-105: Function 'getGates()' should return member 'gates' by const reference.
(returnByReference)
[performance] 112-112: Function 'getNewGates()' should return member 'newGates' by const reference.
(returnByReference)
[style] 113-113: The function 'setCircuitQubit' is never used.
(unusedFunction)
[style] 105-105: The function 'getGates' is never used.
(unusedFunction)
[style] 112-112: The function 'getNewGates' is never used.
(unusedFunction)
[style] 71-71: The function 'toCsv' is never used.
(unusedFunction)
[style] 68-68: The function 'initialCoordinateMappingFromString' is never used.
(unusedFunction)
[style] 91-91: The function 'initialMappingFromString' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
test/hybridmap/test_hybrid_synthesis_map.cpp
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
bindings/hybrid_mapper/hybrid_mapper.cpp
[error] 278-278: Boolean value assigned to pointer.
(assignBoolToPointer)
[style] 276-276: The function 'getOffsetDistance' is never used.
(unusedFunction)
[error] 303-303: Boolean value assigned to pointer.
(assignBoolToPointer)
[error] 303-303: Boolean value assigned to pointer.
(assignBoolToPointer)
[style] 300-300: The function 'getNearbyQubits' is never used.
(unusedFunction)
[style] 301-301: The function 'append' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (14)
test/hybridmap/test_hybrid_synthesis_map.cpp (2)
104-109: LGTM!The
MapAppendtest correctly exercises basic append and synthesis flow.
111-121: LGTM!The
Outputtest correctly uses the updatedappendWithMapping(qc, true)signature and validates QASM output and file operations.src/hybridmap/HybridSynthesisMapper.cpp (3)
70-90: LGTM!The
evaluateSynthesisSteplogic correctly handles thecompleteRemapflag by conditionally preserving mapper state viacopyStateFrom, and properly clones buffered operations in reverse order.
92-120: LGTM!The buffer management logic correctly:
- Auto-initializes mapping when both
mappedQcandbufferedQcare empty- Moves operations from buffer to synthesized circuit when buffer exceeds
bufferSize- Properly erases processed operations from the buffer
The iterator arithmetic with
static_cast<int64_t>is appropriate for signed/unsigned conversion safety.
137-144: LGTM!The structured binding with init-statement (
if (const auto mappedJ = ...; condition)) is a clean C++17 idiom that reduces scope and improves readability.bindings/hybrid_mapper/hybrid_mapper.cpp (3)
230-234: LGTM!The constructor binding correctly exposes the new
buffer_sizeparameter with a sensible default of 0.
270-278: LGTM!The
append_with_mappingbinding correctly exposes thecomplete_remapparameter with defaultfalse, matching the C++ API.
294-307: LGTM!The bindings for
evaluate_synthesis_stepsandcomplete_remapcorrectly expose the new parameters. The static analysis warnings about "Boolean value assigned to pointer" are false positives—this is standard pybind11 keyword argument syntax.include/hybridmap/HybridSynthesisMapper.hpp (6)
73-77: LGTM!The constructor correctly initializes the new
bufferSizemember with a default of 0.
85-95: LGTM!The
initMappingmethod correctly initializes all state including the newbufferedQcand capturesoriginalMappingfor later remap operations.
101-109: LGTM!The
completeRemapmethod correctly handles theincludeBufferflag, usinggetSynthesizedQc()(which includes buffered ops) when true, and onlysynthesizedQcwhen false.
114-124: LGTM!The
schedulemethod correctly:
- Appends buffered operations to
synthesizedQc(unmapped collection)- Maps buffered operations via
mapAppend(tomappedQc)- Clears the buffer before delegating to base scheduler
The duplicate
mapAppendcall from the previous review has been fixed.
130-140: LGTM!
getSynthesizedQccorrectly composes synthesized and buffered operations with proper capacity reservation and deep cloning.
146-162: LGTM!Both OpenQASM export methods correctly use
getSynthesizedQc()to include buffered operations in the output.
lsschmid
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Ludwig Schmid <117631861+lsschmid@users.noreply.github.com>
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @lsschmid. * #885 (comment) The following files were modified: * `include/hybridmap/HybridSynthesisMapper.hpp` * `src/hybridmap/HybridSynthesisMapper.cpp` * `test/hybridmap/test_hybrid_synthesis_map.cpp`
Docstrings generation was requested by @lsschmid. * #885 (comment) The following files were modified: * `include/hybridmap/HybridSynthesisMapper.hpp` * `src/hybridmap/HybridSynthesisMapper.cpp` * `test/hybridmap/test_hybrid_synthesis_map.cpp` <details> <summary>These files were kept as they were</summary> * `bindings/hybrid_mapper/hybrid_mapper.cpp` </details> <details> <summary>ℹ️ Note</summary><blockquote> CodeRabbit cannot perform edits on its own pull requests yet. </blockquote></details>
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)
src/hybridmap/HybridSynthesisMapper.cpp (1)
137-144: LGTM! Clean use of C++17 if-initializer.The structured binding with if-initializer reduces code duplication and improves readability while maintaining the same logic.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hybridmap/HybridSynthesisMapper.cpp(4 hunks)test/hybridmap/test_hybrid_synthesis_map.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cppsrc/hybridmap/HybridSynthesisMapper.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cppsrc/hybridmap/HybridSynthesisMapper.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
src/hybridmap/HybridSynthesisMapper.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/hybridmap/HybridSynthesisMapper.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/hybridmap/HybridSynthesisMapper.cpp
📚 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:
src/hybridmap/HybridSynthesisMapper.cpp
🧬 Code graph analysis (1)
test/hybridmap/test_hybrid_synthesis_map.cpp (2)
test/hybridmap/test_hybridmap.cpp (1)
testArchitecturePath(30-30)include/hybridmap/HybridSynthesisMapper.hpp (2)
qc(61-62)qc(180-181)
🪛 Cppcheck (2.18.0)
test/hybridmap/test_hybrid_synthesis_map.cpp
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
src/hybridmap/HybridSynthesisMapper.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: 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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingInclude)
[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] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 32-32: The function 'evaluateSynthesisSteps' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[performance] 105-105: Function 'getGates()' should return member 'gates' by const reference.
(returnByReference)
[performance] 112-112: Function 'getNewGates()' should return member 'newGates' by const reference.
(returnByReference)
[style] 113-113: The function 'setCircuitQubit' is never used.
(unusedFunction)
[style] 105-105: The function 'getGates' is never used.
(unusedFunction)
[style] 112-112: The function 'getNewGates' is never used.
(unusedFunction)
[style] 71-71: The function 'toCsv' is never used.
(unusedFunction)
[style] 68-68: The function 'initialCoordinateMappingFromString' is never used.
(unusedFunction)
[style] 91-91: The function 'initialMappingFromString' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🔇 Additional comments (4)
test/hybridmap/test_hybrid_synthesis_map.cpp (1)
28-32: LGTM! Test parametrization correctly addresses previous feedback.The test now properly uses
GetParam().completeRemapandGetParam().alsoMapwhen callingevaluateSynthesisSteps, which resolves the previous issue where parametrized test cases were executing identically. The expanded test suite now covers 12 combinations across three architectures with differentcompleteRemapandalsoMapflag combinations.Also applies to: 35-35, 41-41, 76-77, 83-96
src/hybridmap/HybridSynthesisMapper.cpp (3)
32-68: LGTM! Parameter threading is correct.The
completeRemapparameter is properly threaded throughevaluateSynthesisStepsto bothevaluateSynthesisStepandappendWithMapping.
92-120: LGTM! Buffer management and input preservation are correct.The buffer management logic correctly:
- Clones operations from the input
qctobufferedQcwithout modifying the original (lines 99-101)- Moves operations from the beginning of the buffer when
bufferSizeis exceeded (lines 105-109)- Uses
static_cast<int64_t>for safe signed integer arithmetic in the erase operation (line 112)- Conditionally applies complete remapping or incremental mapping based on the parameter (lines 115-119)
This properly addresses the previous concern about modifying the input parameter.
78-86: Operation ordering in synthesis step evaluation needs clarification.The current logic at lines 100-105 reverses
qcCopy, then appends buffered operations in reverse order, producing[op3, op2, op1, buf2, buf1]instead of the expected[buf1, buf2, op1, op2, op3]. The comment "insert buffered operations" contradicts the implementation. Without test coverage or documentation confirming whether the mapper expects reversed circuits, this operation ordering cannot be verified as correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/hybridmap/test_hybrid_synthesis_map.cpp (1)
86-87: LGTM: Past review concern resolved.The test now correctly uses
GetParam().completeRemapandGetParam().alsoMapwhen callingevaluateSynthesisSteps, ensuring that all 12 parametrized test cases execute with distinct configurations. This resolves the previous review comment about unused parameters.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/hybridmap/test_hybrid_synthesis_map.cpp(4 hunks)test/python/hybrid_mapper/test_hybrid_synthesis.py(0 hunks)
💤 Files with no reviewable changes (1)
- test/python/hybrid_mapper/test_hybrid_synthesis.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
test/hybridmap/test_hybrid_synthesis_map.cpp
🧬 Code graph analysis (1)
test/hybridmap/test_hybrid_synthesis_map.cpp (2)
test/hybridmap/test_hybridmap.cpp (1)
testArchitecturePath(30-30)include/hybridmap/HybridSynthesisMapper.hpp (2)
qc(61-62)qc(226-227)
🪛 Cppcheck (2.18.0)
test/hybridmap/test_hybrid_synthesis_map.cpp
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: 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 (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- 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, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (4)
test/hybridmap/test_hybrid_synthesis_map.cpp (4)
28-32: LGTM: Clean test parameterization structure.The TestParams struct effectively organizes the test parameters for comprehensive coverage of architecture types and mapping configurations.
35-51: LGTM: Parametrized test properly updated.The test class correctly adapts to the new TestParams parameter type, and the detailed documentation clearly explains the fixture preparation. The implementation properly accesses
GetParam().architectureto construct the architecture path.
93-106: LGTM: Comprehensive test coverage.The test instantiation systematically covers all combinations of three architectures with both
completeRemapandalsoMapflags, providing thorough validation of the buffer functionality across different architectural configurations.
145-145: LGTM: Output test properly exercises complete remapping.The addition of the
trueparameter explicitly tests the output generation with complete remapping enabled, providing good coverage of the new buffer functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python/mqt/qmap/hybrid_mapper.pyi(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
python/mqt/qmap/hybrid_mapper.pyi (2)
include/hybridmap/HybridSynthesisMapper.hpp (3)
qc(61-62)qc(226-227)filename(203-203)include/hybridmap/HybridNeutralAtomMapper.hpp (9)
qc(549-556)qc(549-550)qc(558-563)qc(558-559)qc(571-571)qc(681-681)filename(612-612)filename(634-634)filename(677-677)
⏰ 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) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc 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 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- 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 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Coverage / 📈 Coverage
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (4)
python/mqt/qmap/hybrid_mapper.pyi (4)
105-105: LGTM! Parameter aligns with C++ implementation.The
complete_remapboolean parameter correctly extends the method signature to enable explicit remapping control. The naming follows Python conventions (snake_case), matching the C++completeRemapparameter from the implementation.
106-106: LGTM! Buffer-aware remapping control added.The
include_bufferparameter appropriately extends thecomplete_remapmethod to control how buffering is considered during remapping, aligning with the PR's buffer functionality objectives.
108-110: LGTM! Evaluation control extended.The
complete_remapparameter addition toevaluate_synthesis_stepsprovides expanded behavior control during synthesis evaluation, matching the C++ implementation'scompleteRemapflag.
102-104: buffer_size parameter properly exposed in Python bindings.The type hints are correct with
typing.SupportsIntappropriately representing the C++uint32_tparameter. Parameter ordering, defaults, and snake_case naming all align with the bindings implementation.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Overall, this looks good to me!
I have a couple of inline comments that should hopefully make sense.
In the bigger picture, I was wondering whether we really need this as additional options, or whether this should simply be the new default?
Are there any downsides to using the new technique?
At some point, having too many options can make the code unnecessarily complicated. Especially if one choice for a parameter is clearly better than another choice.
This is obviously more of a meta comment though.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Ludwig Schmid <117631861+lsschmid@users.noreply.github.com>
|
I'm still working with Korbinian on finalizing the code for the DATE paper. So there might still come some minor changes. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/hybridmap/HybridSynthesisMapper.hpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
include/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-12-10T02:20:01.189Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:0-0
Timestamp: 2025-12-10T02:20:01.189Z
Learning: In QIRProgramBuilder::measure(Value, int64_t), the intentional design prevents duplicate measurements: the early return when `registerResultMap.find(key)` succeeds avoids generating multiple `mz` calls to the same classical bit index and ensures output recording contains only one entry per index. This implements an "override" semantic where repeated measurements to the same resultIndex reuse the cached pointer without additional side effects.
Applied to files:
include/hybridmap/HybridSynthesisMapper.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/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
include/hybridmap/HybridSynthesisMapper.hpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
include/hybridmap/HybridSynthesisMapper.hpp
🧬 Code graph analysis (1)
include/hybridmap/HybridSynthesisMapper.hpp (2)
include/hybridmap/NeutralAtomScheduler.hpp (1)
qc(136-140)include/hybridmap/Mapping.hpp (11)
Mapping(61-61)Mapping(66-70)Mapping(66-66)Mapping(81-107)nodiscard(124-126)nodiscard(135-142)nodiscard(151-159)nodiscard(170-178)nodiscard(187-190)op(199-204)op(199-199)
⏰ 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). (15)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- 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 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (10)
include/hybridmap/HybridSynthesisMapper.hpp (10)
20-20: LGTM: Header includes updated appropriately.The header changes align with the new buffering functionality—
Mapping.hppfor the originalMapping member,NeutralAtomScheduler.hppfor SchedulerResults, and<cstdint>for the uint32_t bufferSize type.Also applies to: 22-22, 26-26
48-50: LGTM: Buffering state members added.The new members (bufferedQc, bufferSize, originalMapping) provide the core state for the buffering functionality. Buffer size enforcement logic is presumably implemented in the source file.
74-81: Consider integrating bufferSize into MapperParameters.The bufferSize is currently a standalone constructor parameter. A previous review comment suggested including it in MapperParameters for consistency with other mapper configuration. This would make the API more uniform and reduce the number of explicit constructor parameters.
Would you like to move bufferSize into the MapperParameters structure, or is there a specific reason to keep it separate?
Based on past review comments, this design question was previously raised but remains unresolved.
86-107: LGTM: Initialization logic is correct.The method properly initializes both the bufferedQc and originalMapping members, and the documentation accurately describes the initialization sequence.
110-128: LGTM: Conditional remapping logic is correct.The method correctly handles both includeBuffer cases. Both branches create temporary circuit copies and invoke map() to update the internal mapping state to originalMapping. The implementation is consistent with the documented behavior.
130-155: LGTM: Scheduling logic correctly integrates buffered operations.The method properly sequences the buffer flush: clones buffered operations into the synthesized circuit, maps and appends them to the mapped circuit, clears the buffer, then delegates to the base scheduler. The previous duplicate mapAppend issue has been resolved.
158-178: LGTM: Combined circuit view implemented correctly.The method properly concatenates synthesized and buffered operations into a single QuantumComputation. The reserve() call optimizes allocation, and the documentation accurately describes the returned combined view.
196-208: LGTM: File export uses combined view.The method correctly exports the combined synthesized+buffered circuit to a file using OpenQASM format. Documentation and implementation are aligned.
210-219: LGTM: Signature updated to support complete remapping.The addition of the completeRemap parameter is documented and consistent with other method updates in this file.
221-227: LGTM: Method signature supports conditional remapping.The completeRemap parameter addition is consistent with the broader API changes and properly documented.
|
Ok, it looks like from our side this seems to be fine. @burgholzer could you have another look regarding your comments? |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
LGTM 👍🏼 Feel free to merge whenever you feel like it.
2 participants
Description
This PR adds buffer functionality to the ZX hybrid synthesis mapper, enabling staged operation management before mapping to hardware. The changes introduce a bufferSize parameter and bufferedQc quantum computation to temporarily store operations, along with support for complete remapping and more flexible synthesis evaluation.
These are the extension which will be presented at DATE 2026
Checklist: