Open
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
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new MLIR Passes module (QcoPasses) including a HeuristicMappingPass, Architecture model (shortest-paths & neighbours), TableGen wiring and CMake integration, implementation sources, and unit tests validating mapping on a RigettiNovera topology. Changes
Sequence Diagram(s)sequenceDiagram
participant MLIR as MLIR Pass Framework
participant HMP as HeuristicMappingPass
participant Arch as Architecture
participant Planner as A* Planner
participant IR as MLIR IR
MLIR->>HMP: runOnOperation()
HMP->>Arch: construct/load architecture
Arch->>Arch: floydWarshallWithPathReconstruction()
HMP->>Planner: initialize root Node with Layout
loop A* search
Planner->>Arch: query distance / adjacency
Planner->>Planner: expand successors (simulate swaps on Layout)
Planner->>HMP: select candidate (routeHot)
HMP->>IR: emit SWAP ops and update mapping
end
HMP->>IR: finalize transformed module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 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 315-319: The error message built in HeuristicMapping.cpp when
dyn.size() > arch.nqubits() concatenates "supports" and the qubit count without
a space; update the emission in the if block (the func.emitError() call) to
include a space after "supports" (or otherwise format the message with
separators) so it prints "the targeted architecture supports 9 qubits, got X";
keep the same use of arch.nqubits(), dyn.size(), and signalPassFailure()
unchanged.
In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 80-87: Duplicate RigettiNovera architecture is defined in
getRigettiNovera() in the test and again in HeuristicMapping.cpp; extract the
shared coupling map into a single source of truth and use it from both places.
Replace the local static COUPLING in getRigettiNovera() by referencing the
shared symbol (or a factory function) you create (e.g., RigettiNoveraCoupling or
makeRigettiNoveraArchitecture) so both the test and HeuristicMapping.cpp use the
same unique identifier; ensure the new symbol returns the same Architecture
(name, qubit count, coupling) and update callers to use that symbol instead of
duplicating the literal list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60e67c87-341c-4173-a238-b33bcd270229
📒 Files selected for processing (2)
mlir/lib/Passes/Mapping/HeuristicMapping.cppmlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
mlir/lib/Passes/Mapping/Architecture.cpp (1)
44-47:⚠️ Potential issue | 🔴 CriticalValidate coupling indices before matrix/vector indexing.
Line 46/Line 47 and Line 72 assume every
(u, v)incouplingSet_is< nqubits_; invalid input causes out-of-bounds access.🛡️ Proposed fix
void Architecture::floydWarshallWithPathReconstruction() { for (const auto& [u, v] : couplingSet_) { + if (u >= nqubits_ || v >= nqubits_) { + report_fatal_error("coupling edge out of range: (" + Twine(u) + ", " + + Twine(v) + ") for architecture size " + + Twine(nqubits_)); + } dist_[u][v] = 1; prev_[u][v] = u; } @@ void Architecture::collectNeighbours() { for (const auto& [u, v] : couplingSet_) { + if (u >= nqubits_ || v >= nqubits_) { + report_fatal_error("coupling edge out of range: (" + Twine(u) + ", " + + Twine(v) + ") for architecture size " + + Twine(nqubits_)); + } neighbours_[u].push_back(v); } }Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Passes/Mapping/Architecture.cpp` around lines 44 - 47, In Architecture::floydWarshallWithPathReconstruction ensure you validate each coupling index before using it: for every (u, v) from couplingSet_ check that 0 <= u < nqubits_ and 0 <= v < nqubits_ (or the equivalent unsigned bounds) before assigning into dist_[u][v] and prev_[u][v]; if an index is out of range, skip that pair (or handle/report it) to prevent out-of-bounds access — apply the same validation wherever couplingSet_ elements are used later in the function (e.g., the loop around lines 70–72).mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp (2)
67-70:⚠️ Potential issue | 🟠 MajorAvoid
DenseMap::operator[]in executability validation.Line 68 and Line 69 can silently insert missing qubit mappings and mask invalid test state. Please assert invariants and use
contains()/lookup().#!/bin/bash rg -n -C3 'op.getNumQubits\(\) > 1|mappings\[op.getQubit' mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp🔧 Proposed fix
+#include <cassert> @@ - if (op.getNumQubits() > 1) { - const auto i0 = mappings[op.getQubit(0)]; - const auto i1 = mappings[op.getQubit(1)]; + if (op.getNumQubits() > 1) { + assert(op.getNumQubits() == 2 && + "Expected only 2-qubit gates after decomposition"); + assert(mappings.contains(op.getQubit(0)) && + "Missing mapping for first qubit"); + assert(mappings.contains(op.getQubit(1)) && + "Missing mapping for second qubit"); + const auto i0 = mappings.lookup(op.getQubit(0)); + const auto i1 = mappings.lookup(op.getQubit(1)); if (!arch.areAdjacent(i0, i1)) { executable = false; return WalkResult::interrupt();Based on learnings: In the MQTOpt MLIR routing passes, input IR is guaranteed to contain only 1-qubit and 2-qubit gates; 3+-qubit gates are invalid before routing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp` around lines 67 - 70, The test uses DenseMap::operator[] on mappings when checking two-qubit op executability (in the block guarded by op.getNumQubits() > 1), which can silently insert missing keys; instead assert the invariant that mappings contains both keys (use mappings.contains(op.getQubit(0)) and mappings.contains(op.getQubit(1))) and fetch values with lookup()/find()/at() (e.g., use mappings.lookup(op.getQubit(0)) and mappings.lookup(op.getQubit(1))). Update the code around op.getNumQubits(), mappings, op.getQubit and arch.areAdjacent to explicitly assert the mapping presence (or fail the test) before calling arch.areAdjacent(i0, i1).
81-87: 🧹 Nitpick | 🔵 TrivialDeduplicate the RigettiNovera coupling map definition.
The topology here duplicates the literal in
mlir/lib/Passes/Mapping/HeuristicMapping.cpp(Line 307-Line 311). Extract a shared factory/source-of-truth to prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp` around lines 81 - 87, The RigettiNovera coupling set in getRigettiNovera() duplicates the literal defined in HeuristicMapping.cpp; refactor by extracting a single shared factory or accessor (e.g., Architecture::rigettiNovera() or a free function createRigettiNoveraArchitecture()) that returns the Architecture with the COUPLING set, replace the local COUPLING and getRigettiNovera() to call that shared factory, and update the occurrence in mlir/lib/Passes/Mapping/HeuristicMapping.cpp to use the same factory so the coupling map is maintained in one place.mlir/lib/Passes/Mapping/HeuristicMapping.cpp (2)
306-311:⚠️ Potential issue | 🟠 MajorHonor the
-archoption instead of hardcoding RigettiNovera.Line 307 ignores user-selected architecture and always routes with RigettiNovera.
🔧 Proposed fix
- // TODO: Hardcoded architecture. + if (!archName.empty() && archName != "RigettiNovera") { + getOperation()->emitError() + << "unsupported architecture '" << archName + << "' (currently only 'RigettiNovera' is implemented)"; + signalPassFailure(); + return; + } Architecture arch("RigettiNovera", 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, The code hardcodes an Architecture instance via Architecture arch("RigettiNovera", ...) which ignores the user-provided -arch option; replace this hardcoded construction with building the Architecture from the parsed CLI option (use the variable or accessor that holds the -arch value, e.g., optArch/archName/archOption used elsewhere in the pass) and/or use the existing architecture factory/lookup routine to create the topology from that option, ensuring the rest of the code references that Architecture instance (Architecture arch) instead of the literal "RigettiNovera".
617-627:⚠️ Potential issue | 🔴 CriticalSet insertion point before creating
SWAPOp.Line 621 emits SWAPs at whatever insertion point was last set, which can misorder operations and violate dominance after
replaceAllUsesExcept.#!/bin/bash rg -n -C8 'for \(const auto \[hw0, hw1\] : \*swaps\)|create<SWAPOp>|setInsertionPoint' mlir/lib/Passes/Mapping/HeuristicMapping.cpp🔧 Proposed fix
const auto unknown = rewriter.getUnknownLoc(); for (const auto [hw0, hw1] : *swaps) { const auto in0 = wires[hw0].qubit(); const auto in1 = wires[hw1].qubit(); + assert(in0.getNumUses() == 1 && in1.getNumUses() == 1 && + "expected linear qubit wires"); + auto* use0 = *in0.getUsers().begin(); + auto* use1 = *in1.getUsers().begin(); + auto* insertBefore = use0->isBeforeInBlock(use1) ? use0 : use1; + rewriter.setInsertionPoint(insertBefore); auto op = rewriter.create<SWAPOp>(unknown, in0, in1); const auto out0 = op.getQubit0Out(); const auto out1 = op.getQubit1Out();🤖 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 617 - 627, The SWAPOp is created at whatever insertion point was last used, which can misorder ops and break dominance after calling rewriter.replaceAllUsesExcept; before calling rewriter.create<SWAPOp>(...) inside the loop over swaps, explicitly set the rewriter insertion point to a deterministic location that dominates the uses (e.g. the block/position where the swap should be placed) so the created SWAPOp is emitted in the correct place; update the loop around wires, swaps, SWAPOp, and replaceAllUsesExcept to call rewriter.setInsertionPoint... (to a dominating op/block for in0/in1) immediately before rewriter.create and then proceed with rewriter.replaceAllUsesExcept.
🤖 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 316-320: The check that compares dyn.size() to arch.nqubits()
emits an error and calls signalPassFailure() but then continues execution,
leading to invalid state and potential out-of-bounds layout access; immediately
return from the enclosing pass handler after calling signalPassFailure() (i.e.,
add a return following signalPassFailure() in the function that contains this
block so no further code runs when func.emitError()/signalPassFailure() is
invoked), referencing the existing dyn, arch, func.emitError(), and
signalPassFailure() symbols to locate the spot to change.
---
Duplicate comments:
In `@mlir/lib/Passes/Mapping/Architecture.cpp`:
- Around line 44-47: In Architecture::floydWarshallWithPathReconstruction ensure
you validate each coupling index before using it: for every (u, v) from
couplingSet_ check that 0 <= u < nqubits_ and 0 <= v < nqubits_ (or the
equivalent unsigned bounds) before assigning into dist_[u][v] and prev_[u][v];
if an index is out of range, skip that pair (or handle/report it) to prevent
out-of-bounds access — apply the same validation wherever couplingSet_ elements
are used later in the function (e.g., the loop around lines 70–72).
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Around line 306-311: The code hardcodes an Architecture instance via
Architecture arch("RigettiNovera", ...) which ignores the user-provided -arch
option; replace this hardcoded construction with building the Architecture from
the parsed CLI option (use the variable or accessor that holds the -arch value,
e.g., optArch/archName/archOption used elsewhere in the pass) and/or use the
existing architecture factory/lookup routine to create the topology from that
option, ensuring the rest of the code references that Architecture instance
(Architecture arch) instead of the literal "RigettiNovera".
- Around line 617-627: The SWAPOp is created at whatever insertion point was
last used, which can misorder ops and break dominance after calling
rewriter.replaceAllUsesExcept; before calling rewriter.create<SWAPOp>(...)
inside the loop over swaps, explicitly set the rewriter insertion point to a
deterministic location that dominates the uses (e.g. the block/position where
the swap should be placed) so the created SWAPOp is emitted in the correct
place; update the loop around wires, swaps, SWAPOp, and replaceAllUsesExcept to
call rewriter.setInsertionPoint... (to a dominating op/block for in0/in1)
immediately before rewriter.create and then proceed with
rewriter.replaceAllUsesExcept.
In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 67-70: The test uses DenseMap::operator[] on mappings when
checking two-qubit op executability (in the block guarded by op.getNumQubits() >
1), which can silently insert missing keys; instead assert the invariant that
mappings contains both keys (use mappings.contains(op.getQubit(0)) and
mappings.contains(op.getQubit(1))) and fetch values with lookup()/find()/at()
(e.g., use mappings.lookup(op.getQubit(0)) and mappings.lookup(op.getQubit(1))).
Update the code around op.getNumQubits(), mappings, op.getQubit and
arch.areAdjacent to explicitly assert the mapping presence (or fail the test)
before calling arch.areAdjacent(i0, i1).
- Around line 81-87: The RigettiNovera coupling set in getRigettiNovera()
duplicates the literal defined in HeuristicMapping.cpp; refactor by extracting a
single shared factory or accessor (e.g., Architecture::rigettiNovera() or a free
function createRigettiNoveraArchitecture()) that returns the Architecture with
the COUPLING set, replace the local COUPLING and getRigettiNovera() to call that
shared factory, and update the occurrence in
mlir/lib/Passes/Mapping/HeuristicMapping.cpp to use the same factory so the
coupling map is maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 013352b5-7595-4d79-bd38-0cb9b8690413
📒 Files selected for processing (4)
mlir/include/mlir/Passes/Mapping/Architecture.hmlir/lib/Passes/Mapping/Architecture.cppmlir/lib/Passes/Mapping/HeuristicMapping.cppmlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp (2)
85-92: 🧹 Nitpick | 🔵 TrivialExtract
RigettiNoveracoupling data to a shared source of truth.This architecture fixture still duplicates production coupling information, which can drift over time and invalidate the test’s meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp` around lines 85 - 92, The test helper getRigettiNovera() is duplicating production coupling data; extract the coupling set into a single shared source of truth and consume that from the test. Replace the inline const static Architecture::CouplingSet COUPLING in getRigettiNovera() with a reference to the canonical definition (e.g., an exported constant or accessor such as Architecture::RigettiNoveraCoupling() or a shared constant RigettiNoveraCoupling) and construct the Architecture("RigettiNovera", 9, ...) using that shared symbol so the test no longer drifts from production data.
67-74:⚠️ Potential issue | 🟠 MajorAvoid mutating
mappingsduring validation by replacingoperator[]withfind/lookup.At Line 72 and Line 73,
DenseMap::operator[]can insert default entries when assertions are disabled, which can silently mask missing mappings in this test helper.Does llvm::DenseMap::operator[] insert a default value when the key is missing, and should lookup/find be preferred for read-only checks?Proposed fix
- assert(mappings.contains(op.getQubit(0)) && "Qubit 0 not in mapping"); - assert(mappings.contains(op.getQubit(1)) && "Qubit 1 not in mapping"); - const auto i0 = mappings[op.getQubit(0)]; - const auto i1 = mappings[op.getQubit(1)]; + const auto it0 = mappings.find(op.getQubit(0)); + const auto it1 = mappings.find(op.getQubit(1)); + assert(it0 != mappings.end() && "Qubit 0 not in mapping"); + assert(it1 != mappings.end() && "Qubit 1 not in mapping"); + if (it0 == mappings.end() || it1 == mappings.end()) { + executable = false; + return WalkResult::interrupt(); + } + const auto i0 = it0->second; + const auto i1 = it1->second;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp` around lines 67 - 74, The test uses mappings[op.getQubit(0)] and mappings[op.getQubit(1)] which can insert default entries when operator[] is used; change these reads to non-mutating lookups (e.g., use mappings.find or mappings.lookup) to avoid modifying mappings during validation: first use mappings.find(op.getQubit(0)) and ensure the iterator is valid (or use lookup and check result) before dereferencing to get i0, do the same for op.getQubit(1) to produce i1, and then call arch.areAdjacent(i0, i1); update the assert messages to reference the lookup result so missing mappings fail the test without mutating the DenseMap.
🤖 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/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 105-109: The tests call runHeuristicMapping() but the
HeuristicMappingPass is hardcoded to RigettiNovera; add a pass option for the
target architecture in the pass definition (Passes.td) and update the
HeuristicMappingPass implementation (HeuristicMapping.cpp) to read that option
instead of using the hardcoded RigettiNovera, then change runHeuristicMapping()
and all its call sites (where createHeuristicMappingPass() is invoked) to pass
the parameterized architecture value (from GetParam().factory() or the arch
argument) into createHeuristicMappingPass() so each test instantiates the pass
with the correct architecture.
---
Duplicate comments:
In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 85-92: The test helper getRigettiNovera() is duplicating
production coupling data; extract the coupling set into a single shared source
of truth and consume that from the test. Replace the inline const static
Architecture::CouplingSet COUPLING in getRigettiNovera() with a reference to the
canonical definition (e.g., an exported constant or accessor such as
Architecture::RigettiNoveraCoupling() or a shared constant
RigettiNoveraCoupling) and construct the Architecture("RigettiNovera", 9, ...)
using that shared symbol so the test no longer drifts from production data.
- Around line 67-74: The test uses mappings[op.getQubit(0)] and
mappings[op.getQubit(1)] which can insert default entries when operator[] is
used; change these reads to non-mutating lookups (e.g., use mappings.find or
mappings.lookup) to avoid modifying mappings during validation: first use
mappings.find(op.getQubit(0)) and ensure the iterator is valid (or use lookup
and check result) before dereferencing to get i0, do the same for op.getQubit(1)
to produce i1, and then call arch.areAdjacent(i0, i1); update the assert
messages to reference the lookup result so missing mappings fail the test
without mutating the DenseMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e958d42-45a0-4252-ab89-cf8bcd7d48d5
📒 Files selected for processing (1)
mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
…ch-quantum-toolkit/core into feat/heuristic-mapping-pass
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp (1)
106-110:⚠️ Potential issue | 🟠 MajorParameterized architecture is not wired into pass execution.
Line 106-Line 110 instantiate
createHeuristicMappingPasswithout any architecture input, while Line 139 and Line 187 callrunHeuristicMapping(module)without passingarch. Givenmlir/lib/Passes/Mapping/HeuristicMapping.cppcurrently hardcodes RigettiNovera, all parameterized cases effectively test the same target architecture.Also applies to: 139-140, 187-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp` around lines 106 - 110, The runHeuristicMapping helper currently constructs the pass without passing any architecture, so all tests use the hardcoded default; modify runHeuristicMapping to accept an architecture parameter (e.g., Arch arch) and forward it into qco::createHeuristicMappingPass by setting the architecture field on qco::HeuristicMappingPassOptions (or using the overload that accepts arch), then update all call sites (the calls to runHeuristicMapping at the test lines indicated) to pass the appropriate arch value so the pass executes against the intended parameterized architecture rather than the hardcoded 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/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 44-47: The test param struct ArchitectureParam currently uses
std::function<Architecture()> for its factory member which is heavier and
non-idiomatic for MLIR tests; change the type of ArchitectureParam::factory to
llvm::function_ref<Architecture()> (or a plain function pointer Architecture
(*)() if you prefer simpler semantics), update the struct definition
(ArchitectureParam) and any places that construct or store factories to use the
new type, and add the required include (llvm/ADT/FunctionRef.h) when using
llvm::function_ref so static factory functions continue to bind correctly.
---
Duplicate comments:
In `@mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp`:
- Around line 106-110: The runHeuristicMapping helper currently constructs the
pass without passing any architecture, so all tests use the hardcoded default;
modify runHeuristicMapping to accept an architecture parameter (e.g., Arch arch)
and forward it into qco::createHeuristicMappingPass by setting the architecture
field on qco::HeuristicMappingPassOptions (or using the overload that accepts
arch), then update all call sites (the calls to runHeuristicMapping at the test
lines indicated) to pass the appropriate arch value so the pass executes against
the intended parameterized architecture rather than the hardcoded RigettiNovera.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64c50248-6961-40ce-a71c-bfe73de4b520
📒 Files selected for processing (1)
mlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mlir/lib/Passes/Mapping/HeuristicMapping.cpp (2)
623-629:⚠️ Potential issue | 🔴 Critical
SWAPOpis created with a stale insertion point.
rewriter.create<SWAPOp>at Line 628 is not preceded by a localsetInsertionPoint(...). This can insert swaps away from the current wire front and break routing correctness.🛠️ Proposed fix
const auto unknown = rewriter.getUnknownLoc(); for (const auto [hw0, hw1] : *swaps) { const auto in0 = wires[hw0].qubit(); const auto in1 = wires[hw1].qubit(); + assert(in0.hasOneUse() && in1.hasOneUse() && + "expected linear qubit wires"); + Operation* use0 = (*in0.use_begin()).getOwner(); + Operation* use1 = (*in1.use_begin()).getOwner(); + assert(use0->getBlock() == use1->getBlock() && + "expected both wire-front uses in same block"); + Operation* insertBefore = + use0->isBeforeInBlock(use1) ? use0 : use1; + rewriter.setInsertionPoint(insertBefore); auto op = rewriter.create<SWAPOp>(unknown, in0, in1); const auto out0 = op.getQubit0Out(); const auto out1 = op.getQubit1Out();🤖 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 623 - 629, The SWAPOp is being created with a stale insertion point: before calling rewriter.create<SWAPOp>(unknown, in0, in1) ensure the rewriter is positioned at the current wire front so swaps are inserted at the correct place. Locate the loop iterating over *swaps that uses wires[hw0].qubit()/wires[hw1].qubit() and before creating SWAPOp call rewriter.setInsertionPoint(...) using the current front operation (e.g., the wire front/op or an available Operation* from wires[hw0]/wires[hw1]) so the newly created SWAPOp and subsequent uses (op.getQubit0Out(), op.getQubit1Out()) are placed correctly relative to the wire front.
305-311:⚠️ Potential issue | 🟠 MajorHardcoded architecture ignores pass configurability.
Line 307 always constructs
Architecture("RigettiNovera", ...), so architecture selection cannot vary per invocation.🔧 Proposed fix
- // TODO: Hardcoded architecture. - Architecture arch("RigettiNovera", 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}}); + const auto selectedArch = + this->archName.empty() ? std::string("RigettiNovera") : this->archName; + if (selectedArch != "RigettiNovera") { + getOperation()->emitError() + << "unsupported architecture '" << selectedArch + << "' (currently only 'RigettiNovera' is implemented)"; + signalPassFailure(); + return; + } + Architecture arch(selectedArch, 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 305 - 311, The code currently constructs a hardcoded Architecture via Architecture arch("RigettiNovera", 9, ...) inside HeuristicMapping, which prevents configuring the target architecture per pass; replace this hardcoded construction with a configurable source (e.g., use an existing pass option or a member like this->architectureName/this->architectureSpec or a function such as getArchitectureFromOptions()) and build Architecture from that input instead of the literal "RigettiNovera" and its edges; update uses of Parameters weights(this->alpha, this->lambda, this->nlookahead) and the location where Architecture arch is constructed so the pass reads the architecture name/size/edges from the pass configuration (or a provided config object) and falls back to a sensible default only if no config is supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mlir/lib/Passes/Mapping/HeuristicMapping.cpp`:
- Around line 623-629: The SWAPOp is being created with a stale insertion point:
before calling rewriter.create<SWAPOp>(unknown, in0, in1) ensure the rewriter is
positioned at the current wire front so swaps are inserted at the correct place.
Locate the loop iterating over *swaps that uses
wires[hw0].qubit()/wires[hw1].qubit() and before creating SWAPOp call
rewriter.setInsertionPoint(...) using the current front operation (e.g., the
wire front/op or an available Operation* from wires[hw0]/wires[hw1]) so the
newly created SWAPOp and subsequent uses (op.getQubit0Out(), op.getQubit1Out())
are placed correctly relative to the wire front.
- Around line 305-311: The code currently constructs a hardcoded Architecture
via Architecture arch("RigettiNovera", 9, ...) inside HeuristicMapping, which
prevents configuring the target architecture per pass; replace this hardcoded
construction with a configurable source (e.g., use an existing pass option or a
member like this->architectureName/this->architectureSpec or a function such as
getArchitectureFromOptions()) and build Architecture from that input instead of
the literal "RigettiNovera" and its edges; update uses of Parameters
weights(this->alpha, this->lambda, this->nlookahead) and the location where
Architecture arch is constructed so the pass reads the architecture
name/size/edges from the pass configuration (or a provided config object) and
falls back to a sensible default only if no config is supplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a6b466e-a3d9-4c92-b5ff-eee8d594fa57
📒 Files selected for processing (2)
mlir/lib/Passes/Mapping/HeuristicMapping.cppmlir/unittests/Passes/Mapping/test_heuristic_mapping.cpp
…ch-quantum-toolkit/core into feat/heuristic-mapping-pass
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
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
1 participant
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: