Merged
Conversation
MatthiasReumann
added
c++
MatthiasReumann
commented
Mar 3, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds a new quantum circuit mapping pass infrastructure to MLIR. It introduces a place-and-route pass for mapping quantum circuits to specific device architectures, including an Architecture class for modeling device connectivity, a Sabre-inspired mapping algorithm with A* search for SWAP insertion, and comprehensive CMake build configuration alongside unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as QC Module
participant QCPass as QCToQCO Pass
participant MappingPass as MappingPass
participant Arch as Architecture Model
participant IRModify as IR Modification
participant QCOPass as QCOToQC Pass
participant Output as QC Module (Mapped)
Input->>QCPass: ModuleOp
QCPass->>Input: Convert to QCO IR
Input->>MappingPass: QCO Module
MappingPass->>Arch: Query device connectivity
Note over MappingPass: Collect dynamic qubits
Note over MappingPass: Compute forward/backward layers
Note over MappingPass: Cold routing (initial placement via A*)
MappingPass->>IRModify: Convert dynamic to static qubits
Note over MappingPass: Hot routing (layer-by-layer)
MappingPass->>Arch: Check qubit adjacency
MappingPass->>IRModify: Insert SWAP operations
MappingPass->>Input: Modified QCO Module
Input->>QCOPass: QCO Module with SWAPs
QCOPass->>Output: Convert back to QC IR
Output->>Output: All 2-qubit gates now adjacent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Compiler/CMakeLists.txt`:
- Line 23: The target currently links QcoPasses as a PUBLIC dependency, exposing
it transitively; change the linkage to PRIVATE so QcoPasses is not exported in
the public interface. Locate the target_link_libraries line that references
QcoPasses in the mlir/lib/Compiler/CMakeLists.txt and replace PUBLIC with
PRIVATE (keeping the same target name and ordering), since public headers under
mlir/include/mlir/Compiler/* do not expose QcoPasses symbols and only
implementation uses passes like QCToQCO, QCOToQC, and QCToQIR.
In `@mlir/lib/Passes/Mapping/Architecture.cpp`:
- Around line 44-47: The loop in Architecture.cpp uses u and v from couplingSet_
as direct indices into dist_, prev_ and neighbours_ without bounds checks;
validate that u and v are within the valid ranges before indexing (e.g., ensure
u < dist_.size() and v < dist_[u].size() and v < prev_[u].size() and u/v <
neighbours_.size() as appropriate), and if an index is out-of-range either skip
the pair (continue) or emit a clear error/diagnostic; apply the same validation
to the other occurrences where couplingSet_ pairs are indexed (the references
around the lines corresponding to the second occurrence at 70-71) and use the
unique symbols couplingSet_, dist_, prev_, neighbours_ to locate and fix all
spots.
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Line 331: Remove the unconditional call to layout.dump() from the normal pass
path and only print when debugging is enabled: locate the call to layout.dump()
in HeuristicMapping (the pass implementation in HeuristicMapping.cpp) and wrap
it with the pass/module debug check (e.g., a conditional using LLVM_DEBUG,
DEBUG_TYPE, or a isDebugEnabled-style flag used in this codebase) so that
layout.dump() executes only under debug logging; ensure you reference the same
debug macro or helper used elsewhere in this file to keep behavior consistent.
- Around line 313-321: The code assumes the number of dynamic qubits (dyn from
collectDynamicQubits) fits the hardware; if dyn.size() > arch.nqubits()
subsequent Layout lookups (Layout::identity, etc.) will index out of range; add
a guard after computing dyn (and before Layout::identity and the SABRE loop)
that checks if dyn.size() > arch.nqubits() and handle it deterministically
(e.g., return a failed LogicalResult/Optional, emit an error/message via the
existing logging/diagnostic mechanism, or early-clamp/resize mappings) so later
uses of layout and any indexing into arch.nqubits()-sized structures are safe;
reference symbols: collectDynamicQubits, dyn, arch.nqubits(), Layout::identity,
and the repeats loop.
- Around line 303-309: The code currently hardcodes the architecture when
constructing Architecture arch(...) which ignores the user-provided -arch
option; update the constructor to use the pass option (e.g. this->archName)
instead of the literal "RigettiNovera" and avoid hardcoding the qubit
count/edges: replace the call Architecture arch("RigettiNovera", 9, {...}) with
a construction that uses the selected architecture name and its associated
topology (for example Architecture arch(this->archName, /*numQubits*/
this->nqubits or query a helper that returns the node count and edge list for
archName) or call a factory like Architecture::fromName(this->archName)),
ensuring the code references the pass field (this->archName) so the -arch option
is honored; keep Parameters weights(this->alpha, this->lambda, this->nlookahead)
as-is.
- Around line 609-620: The SWAPOp is being created with a stale insertion point
because rewriter.create<SWAPOp>(unknown, ...) is called without first moving the
insertion point; before calling rewriter.create in the loop that iterates over
swaps (the block using wires, swaps, in0, in1 and then calling
rewriter.replaceAllUsesExcept), set the rewriter insertion point to the current
wire/front location (e.g., use the appropriate rewriter.setInsertionPoint or
setInsertionPointAfter API on the operation representing the wire front) so the
SWAPOp is inserted at the correct place relative to in0/in1, then create the
SWAPOp and perform replaceAllUsesExcept as shown.
- Line 478: The variable "step" is declared with the unsigned type std::size_t
but needs to hold negative values for Direction::Backward; change its type to a
signed pointer-difference type (e.g., std::ptrdiff_t) so it matches
WireIterator::difference_type and avoids underflow when set to -1; update the
declaration near the constexpr std::size_t step = d == Direction::Forward ? 1 :
-1; (used with std::ranges::advance and wire iteration) to use std::ptrdiff_t
(or WireIterator::difference_type) instead.
In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Line 135: Remove the unconditional IR dumps from the test by deleting or
gating the module->dump() calls so they do not print on every successful run;
locate the invocations of module->dump() in test_heuristic_mapping.cpp (the
calls at and near the two occurrences noted) and either remove them or wrap them
behind a debug/verbose guard (e.g., LLVM_DEBUG/if (verbose)) so CI output is not
noisy while preserving the ability to enable dumps for debugging.
- Around line 66-73: The current code uses mappings[op.getQubit(...)] which can
insert default entries and only checks op.getNumQubits() > 1; change this to
defensively lookup qubit indices (e.g., use mappings.find or contains) for
op.getQubit(0) and op.getQubit(1), verify both exist before using them, and
explicitly require op.getNumQubits() == 2 (reject ops with 0, 1, or >2 qubits) —
if a lookup fails or the arity is not 2, set executable = false and return
WalkResult::interrupt() (retain checks using arch.areAdjacent(i0, i1) only after
successful lookups).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dccba142-3321-49bd-bcfa-8639de6bb40c
📒 Files selected for processing (14)
mlir/include/mlir/CMakeLists.txtmlir/include/mlir/Passes/CMakeLists.txtmlir/include/mlir/Passes/Mapping/Architecture.hmlir/include/mlir/Passes/Passes.hmlir/include/mlir/Passes/Passes.tdmlir/lib/CMakeLists.txtmlir/lib/Compiler/CMakeLists.txtmlir/lib/Passes/CMakeLists.txtmlir/lib/Passes/Mapping/Architecture.cppmlir/lib/Passes/Mapping/HeuristicMapping.cppmlir/unittests/CMakeLists.txtmlir/unittests/Passes/CMakeLists.txtmlir/unittests/Passes/Mapping/CMakeLists.txtmlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
mlir/lib/Passes/Mapping/HeuristicMapping.cpp (1)
306-311:⚠️ Potential issue | 🟠 MajorHonor the
-archoption instead of hardcodingRigettiNovera.Line 307 ignores user configuration and makes pass behavior non-configurable.
🔧 Proposed fix
- // TODO: Hardcoded architecture. - Architecture arch("RigettiNovera", 9, + if (!archName.empty() && archName != "RigettiNovera") { + getOperation()->emitError() + << "unsupported architecture '" << archName + << "' (currently only 'RigettiNovera' is implemented)"; + signalPassFailure(); + return; + } + Architecture arch(archName.empty() ? "RigettiNovera" : archName, 9, {{0, 3}, {3, 0}, {0, 1}, {1, 0}, {1, 4}, {4, 1}, {1, 2}, {2, 1}, {2, 5}, {5, 2}, {3, 6}, {6, 3}, {3, 4}, {4, 3}, {4, 7}, {7, 4}, {4, 5}, {5, 4}, {5, 8}, {8, 5}, {6, 7}, {7, 6}, {7, 8}, {8, 7}});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp` around lines 306 - 311, Replace the hardcoded Architecture("RigettiNovera", ...) in HeuristicMapping.cpp with code that reads and uses the -arch option value (or the pass's configured architecture parameter) to construct the Architecture instance; locate the symbol Architecture arch(...) and change it to create the architecture from the pass option (e.g., query the existing -arch CLI/pass option or injected config, map that name to the topology data, and then construct Architecture using that name and its connectivity) so the pass honors user configuration instead of always using "RigettiNovera".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Line 23: Remove the unused include header by deleting the line that includes
TopologicalSortUtils.h in HeuristicMapping.cpp; locate the include directive
"#include <mlir/Analysis/TopologicalSortUtils.h>" near the top of the file (in
the compilation unit that defines the HeuristicMapping pass) and remove it so
only required headers remain, then rebuild to confirm no missing references in
functions/classes within that file.
- Around line 314-345: Before attempting placement/routing, skip functions that
the mapper cannot handle: detect and continue past declarations or bodies with
unsupported shapes by checking
func.isDeclaration()/func.getFunctionBody().empty() (or that the body does not
consist of exactly one block) and validate the wire shape returned by
collectDynamicQubits is compatible (e.g., non-empty and represents a
linear/contiguous set). If a function is unsupported emit a non-fatal diagnostic
on the FuncOp and continue the loop instead of calling place,
computeBidirectionalLayers, routeCold, or routeHot; keep existing error paths
for true mapping failures but avoid dereferencing front()/back() on
empty/structured bodies.
- Line 299: The alias MinQueue currently uses std::vector as the underlying
container which is inconsistent with project conventions; change the backing
container to llvm::SmallVector for the alias named MinQueue (i.e. replace
std::vector<Node> with SmallVector<Node, <appropriate inline capacity>>),
keeping the comparator std::greater<> and the Node element type unchanged so the
new typedef reads the same except for the container type; pick a sensible inline
capacity (e.g., 8 or matching nearby usages) and ensure SmallVector is in scope
where MinQueue is declared.
---
Duplicate comments:
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Around line 306-311: Replace the hardcoded Architecture("RigettiNovera", ...)
in HeuristicMapping.cpp with code that reads and uses the -arch option value (or
the pass's configured architecture parameter) to construct the Architecture
instance; locate the symbol Architecture arch(...) and change it to create the
architecture from the pass option (e.g., query the existing -arch CLI/pass
option or injected config, map that name to the topology data, and then
construct Architecture using that name and its connectivity) so the pass honors
user configuration instead of always using "RigettiNovera".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 429a91b3-4fcd-4a09-9397-fb74f4e44bbb
📒 Files selected for processing (1)
mlir/lib/Passes/Mapping/HeuristicMapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Around line 284-295: The cost function h computes nswaps as
arch.distanceBetween(hw0, hw1) - 1 but assumes hw0 != hw1; add a defensive check
before using distanceBetween: after calling layout.getHardwareIndices(prog0,
prog1) in h, assert hw0 != hw1 (or if assertions are disabled,
early-continue/skip this pair or handle as zero-cost) to avoid underflow when
prog0==prog1 or both map to the same hardware qubit; reference symbols: function
h, layout.getHardwareIndices, arch.distanceBetween, params.decay, and the loop
over layers.
- Around line 394-402: The loop variable p is misleading because it iterates
synthetic hardware positions, not program qubit indices; rename p to something
clearer (e.g., hwPos or hwIdx) and add a brief comment above the loop explaining
that the range [dynQubits.size(), layout.nqubits()) represents synthetic
hardware slots to be filled with StaticOp instances. Update uses of p in the
loop (the call to layout.getHardwareIndex(p), creation of StaticOp/DeallocOp,
and assignment to statics[hw]) to the new name and keep funcBody, StaticOp,
DeallocOp, dynQubits and statics references intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95719ba4-6bb4-4b67-b8ff-70533c8d9a46
📒 Files selected for processing (1)
mlir/lib/Passes/Mapping/HeuristicMapping.cpp
burgholzer
requested changes
Mar 6, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This looks fantastic. I only have minor comments that should be easily addressed.
The PR should be added to the list of PRs on the changelog as well.
Let's try to get this in quickly to make further advances on the methods here in follow-up PRs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
mlir/lib/Passes/Mapping/Mapping.cpp (1)
311-343:⚠️ Potential issue | 🔴 CriticalSkip functions the mapper cannot legally rewrite.
This loop currently tries to map every
func::FuncOp. IfcollectDynamicQubits()is empty,place()still pads the function toarch.nqubits()statics/deallocs; if the body is a declaration, empty, or uses unsupported control flow, later code either dereferences invalid region endpoints or aborts through the wire-traversal fallback. Please bail out early for non-quantum functions and emit a normal pass failure for unsupported IR shapes before entering placement/routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Passes/Mapping/Mapping.cpp` around lines 311 - 343, Skip non-quantum or unsupported functions early: inside the loop over getOperation().getOps<func::FuncOp>(), after calling collectDynamicQubits(func.getFunctionBody()) check for empty or otherwise-unsupported IR shapes (e.g., function is a declaration, body is empty, or uses unsupported control flow) and bail out before computing computeBidirectionalLayers, placement, or routing; emit a user-facing error via func.emitError() (or call signalPassFailure() for a normal pass failure) when the IR shape is unsupported, and return so you never call place(), routeCold(), or routeHot() on functions that cannot be legally rewritten.mlir/include/mlir/Passes/Passes.td (1)
18-33:⚠️ Potential issue | 🟠 MajorExpose the target architecture in the pass interface.
The contract here says “given architecture”, but the only public options are heuristic tunables. That forces
mlir/lib/Passes/Mapping/Mapping.cppto hardcode RigettiNovera and leavesmlir/unittests/Passes/Mapping/test_mapping.cpp:84-91with a second copy of the topology. Please add an architecture/coupling-map option here, or narrow the pass contract to RigettiNovera-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Passes/Passes.td` around lines 18 - 33, The MappingPass declaration lacks a public option for selecting the target architecture/coupling map, forcing consumers (mlir/lib/Passes/Mapping/Mapping.cpp and tests like mlir/unittests/Passes/Mapping/test_mapping.cpp) to hardcode topologies; add a new Option in the MappingPass options list (e.g., Option<"architecture", "architecture", "std::string", "\"RigettiNovera\"", "The target architecture or coupling-map identifier."> or an Option named "coupling_map" with a string or serialized topology default) so the pass exposes the architecture selection; update Mapping.cpp to read the generated pass option (the new architecture/coupling_map option) instead of hardcoding RigettiNovera and adjust tests to pass the desired architecture via the pass option.
🤖 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 15-16: Add an explicit bullet under the "### Added" section in
CHANGELOG.md calling out PR `#1537` and describing the new mapping/place-and-route
pass and architecture support (e.g., "Added new place-and-route mapping pass and
architecture support (see `#1537`)"); locate the generic QC/QCO infrastructure
entry that currently lists PRs including 1537 and remove 1537 from that generic
line so the user-facing feature is discoverable and clearly attributed to the
new bullet.
In `@mlir/lib/Passes/Mapping/Mapping.cpp`:
- Around line 302-304: In runOnOperation(), validate that the alpha parameter is
strictly positive before constructing Parameters or calling search(): check
this->alpha (or the value passed into Parameters) and if alpha <= 0 emit a clear
failure (fail fast) and abort the pass rather than proceeding; mention the check
in the same function that creates Parameters (runOnOperation) and ensure
search() is never invoked when alpha <= 0 to prevent equal-cost or negative-cost
cycling.
---
Duplicate comments:
In `@mlir/include/mlir/Passes/Passes.td`:
- Around line 18-33: The MappingPass declaration lacks a public option for
selecting the target architecture/coupling map, forcing consumers
(mlir/lib/Passes/Mapping/Mapping.cpp and tests like
mlir/unittests/Passes/Mapping/test_mapping.cpp) to hardcode topologies; add a
new Option in the MappingPass options list (e.g., Option<"architecture",
"architecture", "std::string", "\"RigettiNovera\"", "The target architecture or
coupling-map identifier."> or an Option named "coupling_map" with a string or
serialized topology default) so the pass exposes the architecture selection;
update Mapping.cpp to read the generated pass option (the new
architecture/coupling_map option) instead of hardcoding RigettiNovera and adjust
tests to pass the desired architecture via the pass option.
In `@mlir/lib/Passes/Mapping/Mapping.cpp`:
- Around line 311-343: Skip non-quantum or unsupported functions early: inside
the loop over getOperation().getOps<func::FuncOp>(), after calling
collectDynamicQubits(func.getFunctionBody()) check for empty or
otherwise-unsupported IR shapes (e.g., function is a declaration, body is empty,
or uses unsupported control flow) and bail out before computing
computeBidirectionalLayers, placement, or routing; emit a user-facing error via
func.emitError() (or call signalPassFailure() for a normal pass failure) when
the IR shape is unsupported, and return so you never call place(), routeCold(),
or routeHot() on functions that cannot be legally rewritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56ed9d50-b2c0-4df4-82f4-a58073b7a9bd
📒 Files selected for processing (5)
CHANGELOG.mdmlir/include/mlir/Passes/Passes.tdmlir/lib/Passes/Mapping/Mapping.cppmlir/unittests/Passes/Mapping/CMakeLists.txtmlir/unittests/Passes/Mapping/test_mapping.cpp
burgholzer
requested changes
Mar 9, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Okay, I only have nitpicks or comments for the future left. Let's give this one more quick round and then let's get it in.
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
approved these changes
Mar 9, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just pushed a couple of refinements in the last commits. @MatthiasReumann please have a look. This should be good to go from my side now.
Member
|
✅ all systems green, let's go 🚀 |
Collaborator
Author
|
@burgholzer fyi: I added one more commit changing the last |
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
This pull request reimplements the heuristic mapping pass of the old mqtopt dialect. Similarly to #1510 the mapping pass ignores structured control flow (for now). I've copied the CMake setup of #1206.
A key difference between the implementations is that here I avoid building the "fat" Layout object (which keeps track of the SSA values) and use the
WireIterators and the "lightweight" Layout instead. Moreover, just because I wanted to highlight how cool theWireIteratoris, this pull request adds LightSABRE-like back and forth traversal to find a better initial layout.Lastly, I think it should be possible to use the ideas implemented here for a unitized IR also.
Checklist: