✨ Implement Trials For Mapping Pass#1568
Merged
burgholzer merged 10 commits intomainfrom Mar 18, 2026
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds randomized multi-trial mapping to the QCO place-and-route pass via two new pass options ( Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant Pass as MappingPass
end
rect rgba(200,255,200,0.5)
participant RNG as RandomEngine
participant Factory as TrialFactory
end
rect rgba(255,200,200,0.5)
participant Mapper as MappingRoutine
participant Selector as ResultSelector
participant IR as IRCommitter
end
Pass->>RNG: init(seed)
Pass->>Factory: build(identity + nrandom(ntrials))
Factory-->>Pass: trial layouts
par parallel trials
Pass->>Mapper: runMappingTrial(layout)
Mapper->>Mapper: perform routing, collect SWAPs
Mapper-->>Pass: TrialResult(layout, swaps)
end
Pass->>Selector: findBestTrial(all TrialResult)
Selector-->>Pass: BestTrialResult
Pass->>IR: commitTrial(BestTrialResult)
IR-->>Pass: IR updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 53-54: Update the documentation for the pass option `ntrials` in
Passes.td to explicitly state that the pass always evaluates the identity layout
in addition to the randomized trials (i.e., `ntrials` specifies the number of
additional random trials), so that `ntrials = 0` still runs the identity
baseline; make the same wording change at the second occurrence noted around
lines 65-66 so both descriptions consistently document that the identity layout
is evaluated in addition to the `ntrials` random trials.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 101-104: The code uses std::iota and std::optional but relies on
transitive includes; add direct includes for <numeric> and <optional> to the top
of the file so these symbols are guaranteed available. Update the include list
in mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (affecting uses in the
static function Layout random(...) and the other occurrence around line 361) by
adding `#include` <numeric> and `#include` <optional> to ensure std::iota and
std::optional are provided explicitly.
In `@mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp`:
- Around line 107-111: The test calls qco::createMappingPass with a
MappingPassOptions struct but doesn’t set the randomized seed, so tests can
become nondeterministic if the default changes; update the MappingPassOptions
initializer used in the test (the call to qco::createMappingPass /
MappingPassOptions in test_mapping.cpp) to include an explicit .seed value
(e.g., a fixed integer) alongside .nlookahead, .alpha, .lambda, .niterations,
and .ntrials to make the mapping trials deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51b35893-e79e-485d-a7e6-a308235085cd
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
MatthiasReumann
added
c++
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
326-356: 🧹 Nitpick | 🔵 TrivialMake the seeded trials reproducible per function.
Line 326 seeds a single
std::mt19937_64before the function loop, so the trial sequence for a givenfunc::FuncOpchanges when unrelated functions are added or reordered earlier in the module. Sinceseedis now a user-facing pass option, it would be better to derive a local RNG inside the loop from(this->seed, function identity/index)and keep each function stable on its own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp` around lines 326 - 356, The RNG is currently seeded once (std::mt19937_64 rng{this->seed}) outside the func::FuncOp loop, causing the sequence of Layout::random trials for a given function to shift when other functions change; fix by deriving a per-function RNG inside the loop using the pass seed combined with a stable function identifier (e.g., func.getName().str() or an incrementing function index). Replace the single outer rng with creating a local std::mt19937_64 (or seeded via std::seed_seq) for each func (use this->seed plus a hash/uint32 of func identity) and use that local rng when calling Layout::random so each func::FuncOp produces reproducible trials independent of module ordering.
🤖 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/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 351-356: The current trials use Layout::random(arch.nqubits(),
rng()) which permutes all program indices including synthetic suffix
[dyn.size(), arch.nqubits()), wasting trials; change Layout::random (or add an
overload) so it only shuffles the first dynCount program slots (use dyn.size()
as the count) and then deterministically assigns the remaining hardware slots to
the synthetic program indices, and update the call in Mapping.cpp (where
trials.emplace_back(...) is used) to pass dyn.size() (or call the new overload)
so only real dynamic qubits are randomized while synthetic indices remain fixed.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 326-356: The RNG is currently seeded once (std::mt19937_64
rng{this->seed}) outside the func::FuncOp loop, causing the sequence of
Layout::random trials for a given function to shift when other functions change;
fix by deriving a per-function RNG inside the loop using the pass seed combined
with a stable function identifier (e.g., func.getName().str() or an incrementing
function index). Replace the single outer rng with creating a local
std::mt19937_64 (or seeded via std::seed_seq) for each func (use this->seed plus
a hash/uint32 of func identity) and use that local rng when calling
Layout::random so each func::FuncOp produces reproducible trials independent of
module ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d3abc37-5aaa-4d00-8c88-c71c1007233c
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 532-535: The loop condition uses the comma operator so
!frontier.empty() is discarded and i < maxIterations is the only check, which
can call frontier.top() on an empty queue; change the while condition to require
both !frontier.empty() && i < maxIterations so the loop stops when the frontier
is empty or the iteration limit is reached (update the loop controlling
expression where variables frontier, i, and maxIterations are used before
calling frontier.top()/frontier.pop()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 071bea0f-49fc-405f-8611-a9f097c6c7d5
📒 Files selected for processing (4)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
burgholzer
approved these changes
Mar 17, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This looks very clean to me. I only had one minor observation.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
101-112: 🧹 Nitpick | 🔵 TrivialRandomization includes synthetic qubit indices.
The
Layout::random(nqubits, seed)shuffles allnqubitsindices uniformly. Whendyn.size() < arch.nqubits(), the synthetic program indices in[dyn.size(), arch.nqubits())are also permuted. Since these never affect routing quality, equivalent placements of the actual qubits are counted as distinct trials.Consider restricting the shuffle to only the first
dyn.size()indices, assigning the remaining hardware slots deterministically. This would make better use of the trial budget for narrow circuits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp` around lines 101 - 112, Layout::random currently shuffles all nqubits including synthetic program indices, causing equivalent placements to be counted multiple times; change it to only randomize the first dyn.size() program indices and leave the remaining hardware slots deterministic. Concretely, in Layout::random(const std::size_t nqubits, const std::size_t seed) (and any callers that pass arch.nqubits()/dyn.size()), build the mapping so that you fill indices [0, dyn.size()) then apply std::ranges::shuffle or mt19937_64 only to that prefix, and then call Layout::add(prog, hw) via the enumerate(mapping) loop so that program qubits are permuted but the synthetic indices in [dyn.size(), arch.nqubits()) remain in a fixed order.
🤖 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/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 101-112: Layout::random currently shuffles all nqubits including
synthetic program indices, causing equivalent placements to be counted multiple
times; change it to only randomize the first dyn.size() program indices and
leave the remaining hardware slots deterministic. Concretely, in
Layout::random(const std::size_t nqubits, const std::size_t seed) (and any
callers that pass arch.nqubits()/dyn.size()), build the mapping so that you fill
indices [0, dyn.size()) then apply std::ranges::shuffle or mt19937_64 only to
that prefix, and then call Layout::add(prog, hw) via the enumerate(mapping) loop
so that program qubits are permuted but the synthetic indices in [dyn.size(),
arch.nqubits()) remain in a fixed order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2c17013-65af-45cf-892f-86c51dcc5008
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Architecture.hmlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cpp`:
- Line 16: The loop that computes the maximum degree using std::max should be
replaced with LLVM's helper to follow mlir conventions: locate the code in
Architecture.cpp (the max-degree reduction loop near lines 77-80, e.g., inside
the function that computes maxDegree or similar) and replace the manual
std::max-based iteration with llvm::max_element (from llvm/ADT/STLExtras.h) over
the container holding degrees, then dereference the returned iterator to get the
max value; also add the appropriate LLVM header include and remove the
unnecessary <algorithm> use for this purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b872f515-e784-4715-8ffb-740d3ad85bd6
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cpp
burgholzer
approved these changes
Mar 18, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Very clean. Let's get this in before conflicts creep up.
This was referenced Mar 24, 2026
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 adds LightSABRE-like trials (configurable via pass options) to improve the mapping result. To avoid that the A* search gets stuck, this pull request also adds a configurable
maxIterationsparameter.Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.