✨ Improve Mapping Pass#1600
Merged
MatthiasReumann merged 76 commits intomainfrom May 6, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors the QCO mapping pass pipeline by introducing new public APIs ( ChangesQubit Mapping Pipeline Refactor
Sequence Diagram(s)sequenceDiagram
participant Pass as MappingPass
participant Routing as Window-based<br/>A* Router
participant Placement as Placement &<br/>Commit
participant Validation as isExecutable
Pass->>Routing: Forward routing:<br/>build window, A* search, apply SWAPs
activate Routing
Routing->>Routing: Repeat for backwards pass
deactivate Routing
Routing-->>Pass: Updated IR with<br/>SWAPs committed
Pass->>Placement: Rewrite dynamic to<br/>static qubits
activate Placement
Placement->>Placement: Handle tensor alloc/dealloc,<br/>topological sort
deactivate Placement
Placement-->>Pass: Mapped function
Pass->>Validation: Validate mapped<br/>program feasibility
activate Validation
Validation->>Validation: Walk region, check<br/>two-qubit gate adjacency
deactivate Validation
Validation-->>Pass: LogicalResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/include/mlir/Dialect/QCO/Utils/Drivers.h`:
- Around line 63-66: The Doxygen `@returns` for getHardwareIndex is wrong (it
describes the inverse); update the comment to state that given a
TypedValue<QubitType> q it returns the corresponding hardware index
(std::size_t). Locate the getHardwareIndex(TypedValue<QubitType> q) declaration
and replace the incorrect "@returns the qubit value assigned to a hardware
index" text with a correct description like "@returns the hardware index
assigned to the given qubit" (mirroring the intent of getHardwareIndex and
avoiding the getHardwareQubit wording).
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 78-79: Wrap the unconditional llvm::dbgs() debug prints for the
"not adjacent: " message and op->getLoc() in the LLVM_DEBUG macro so they only
emit in debug builds; specifically replace the two lines that print i0, i1 and
op->getLoc() with a single LLVM_DEBUG block that calls llvm::dbgs() << ... for
both messages (referencing the existing symbols i0, i1 and op), ensuring the
code compiles with existing DEBUG_TYPE/LLVM_DEBUG usage in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bf1ddf8-6500-4a1e-a049-9c774137d817
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Driver.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: 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/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 862-868: The partitioning loop in partitionLayers(ArrayRef<Layer>
layers, const std::size_t sz) advances by sz, so if a user sets
partition-size-forward or partition-size-backward to 0 the loop becomes
infinite; reject zero-sized partitions up front by validating sz != 0 and
failing fast (e.g., return an error, emit a diagnostic, or assert) before
entering the for-loop in partitionLayers (and apply the same guard to the
similar partitioning helper used around lines 879-888); include a clear error
message that the partition size must be >= 1.
- Around line 64-75: The verifier is calling the assert-only
Qubits::getHardwareIndex unconditionally inside isExecutable (during the
UnitaryOpInterface handling for q0/q1), which can abort in debug or misreport in
release for qco.alloc-origin qubits; change the logic to first check whether a
hardware mapping exists for each operand (e.g. use a safe query like
qubits.hasHardwareIndex or an equivalent try-get API) before calling
getHardwareIndex, and if a mapping is absent return failure()/interrupt the walk
(so isExecutable returns failure) rather than calling the assert-only accessor;
update the code paths around isExecutable, the walkUnit lambda, and the q0/q1
handling to use the safe check and early-fail behavior.
In `@mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp`:
- Around line 105-106: The test is calling isExecutable on
moduleOp->getBodyRegion(), which only sees top-level func.func ops and never
inspects the function internals; update both assertions (the calls after
runHeuristicMapping) to point at the function's body region instead of the
module body: locate the func::FuncOp produced in the test (or get the first
function from moduleOp, e.g., the func op created earlier) and call
isExecutable(funcOp.getBodyRegion(), arch) so the helper walks the mapped
quantum ops inside the function body; apply this change to both occurrences that
currently use moduleOp->getBodyRegion().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0055e081-a564-45ec-a147-f18d80544440
📒 Files selected for processing (4)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
requested changes
Apr 1, 2026
Member
There was a problem hiding this comment.
Thanks @MatthiasReumann for another PR that improves the performance of the mapping pass 🙏🏼
I pushed a couple of smaller fixes and code quality improvements; nothing too major.
However, I couldn't help but notice that the code is becoming more and more complex with every one of these PRs and it is no longer as neat and tidy as it once was. Probably a consequence of the hectic of the last couple of weeks. Before we move on with this here, can we take one step back and reflect whether this is still heading in the right direction?
One of the things that I frequently stumbled over when looking through the code of the PR is that we currently generate all layers of the program upfront, even though we only use a finite lookahead in the actual routing process.
This feels like a huge memory overhead (given that the original program has to be kept in memory and a duplicated layered view). Is this something that we could potentially revisit?
I am also not sure that I am a big fan of the new layer splitting options. When would I ever make a difference between the forward and the backwards pass? Isn't one of the options (either partitioning or not) always superior to the other? Should this even be an option.
I hope you see where my thoughts are going. This is getting more complicated with every pull request; even though one of our main points was that MLIR makes things simpler.
Edit: I also feel like the current code creates quite a few copy of things because it frequently materializes some ranges, which might in the worst case be ranges over operations of the entire program. I might be wrong on this one though; it just felt like it from reading the code.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/Mapping/Mapping.h`:
- Around line 33-35: Add a Doxygen comment block above the createMappingPass
declaration that includes a brief description and `@param/`@return tags: document
the purpose of createMappingPass, describe the arch parameter
(std::shared_ptr<Architecture> arch) and the options parameter
(MappingPassOptions options) including default behavior, and document the return
value (std::unique_ptr<Pass>) indicating ownership and what the created Pass
does. Ensure the block uses standard Doxygen tags (brief, `@param`, `@return`) and
matches existing header documentation style.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 660-662: The loop that advances curr1 is using the wrong
predicate: it checks Traits::isActive(curr0) while advancing curr1, so curr1
never skips its own leading single-qubit ops; update the second alignment loop
in skipQubitPairBlock (the loop using Traits::isActive and std::ranges::advance
on curr1 with Traits::stride()) to check Traits::isActive(curr1) instead of
Traits::isActive(curr0) so curr1 advances according to its own active state and
stride.
- Around line 408-417: When route<WireDirection::Forward, RoutingMode::Hot>(...)
fails (failed(res) is true) the code currently calls signalPassFailure() but
continues and dereferences *res; change the control flow so you immediately
return/exit the pass after logging the error (func.emitError()) and calling
signalPassFailure() to avoid dereferencing res and running the later
stats/asserts; ensure the subsequent uses (numSwaps = *res and any
reordering/asserts) only run when res succeeded.
- Around line 347-352: The MappingPass constructors currently create a default
empty Architecture and runOnOperation seeds layouts using arch->nqubits()
without validating that the target function/module fits; update MappingPass
(both the no-arg and MappingPass(MappingPassOptions) constructors or right
before seeding in runOnOperation) to check the module/function logical qubit
count against arch->nqubits() and BailOut/fail the pass with a clear diagnostic
if the architecture is too small (so Layout::getHardwareIndex() in
place()/routing cannot be reached); reference MappingPass, runOnOperation,
arch->nqubits(), Layout::getHardwareIndex(), place(), and routing when adding
the guard and error report.
In `@mlir/lib/Dialect/QCO/Utils/Qubits.cpp`:
- Around line 72-75: Add a precondition check in Qubits::getIndex to assert the
qubit exists before calling valueToIndex_.lookup; mirror the pattern used in
Qubits::remap, remove, getProgramQubit and getHardwareQubit by verifying
membership (e.g., calling the same contains/has method or checking valueToIndex_
for presence) and failing fast with an assertion if the TypedValue<QubitType> is
missing, then proceed to call valueToIndex_.lookup(q) and return res.second.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a67b0ecf-5b91-4707-b87b-1c9e5abe2b44
📒 Files selected for processing (12)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Architecture.hmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/include/mlir/Dialect/QCO/Utils/Qubits.hmlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Qubits.cppmlir/lib/Dialect/QCO/Utils/WireIterator.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
11 tasks
jmoosburger
pushed a commit
to jmoosburger/mqt-core
that referenced
this pull request
Apr 30, 2026
This pull request adds the `walkProgramGraph` driver functionality originally developed in munich-quantum-toolkit#1600 in an effort to reduce the PR size there. Moreover, munich-quantum-toolkit#1661 also depends on the driver. <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [X] The pull request only contains commits that are focused and relevant to this change. - [X] I have added appropriate tests that cover the new/changed functionality. - [X] I have updated the documentation to reflect these changes. - [x] I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals. - [X] I have added migration instructions to the upgrade guide (if needed). - [X] The changes follow the project's style guidelines and introduce no new warnings. - [X] The changes are fully tested and pass the CI checks. - [X] I have reviewed my own code changes. **If PR contains AI-assisted content:** - [X] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [X] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [X] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. --------- Signed-off-by: burgholzer <burgholzer@me.com> Co-authored-by: Copilot <copilot@github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: burgholzer <burgholzer@me.com>
burgholzer
added a commit
that referenced
this pull request
May 4, 2026
## Description While looking at the pass I noticed an opportunity to utilize the wire iterator. Hence, this pull request. Cherry-picks the wire iterator changes introduced in #1600 to support tensors. ## Checklist <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [X] The pull request only contains commits that are focused and relevant to this change. - [X] I have added appropriate tests that cover the new/changed functionality. - [X] I have updated the documentation to reflect these changes. - [x] I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals. - [X] I have added migration instructions to the upgrade guide (if needed). - [X] The changes follow the project's style guidelines and introduce no new warnings. - [X] The changes are fully tested and pass the CI checks. - [X] I have reviewed my own code changes. **If PR contains AI-assisted content:** - [X] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [X] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [X] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. --------- Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: burgholzer <burgholzer@me.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Co-authored-by: burgholzer <burgholzer@me.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 410-415: The diagnostic is using func->getName() (OperationName)
instead of the function symbol; change the error message to use func.getName()
so the symbol identifier is printed. Locate the block that checks failed(res)
and update the emitError call (func.emitError()) to reference func.getName() for
the name shown, keeping the rest of the control flow (signalPassFailure();
return;) unchanged.
- Around line 469-483: The code dereferences
rewriter.getInsertionPoint()->getLoc() which can be invalid after erasing ops;
change to use a stable location (func.getLoc()) when creating StaticOp and
SinkOp: in the loop that creates static qubits (references:
rewriter.getInsertionPoint(), StaticOp::create, SinkOp::create,
func.getFunctionBody(), layout.getHardwareIndex, layout.nqubits(), and index p)
replace the use of the insertion-point-derived location with func.getLoc() and
keep the insertion-point manipulations (setInsertionPointToStart /
setInsertionPoint(...getTerminator())) only for positioning the new ops, not for
obtaining their Location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 820ec7d6-5f75-4729-8479-8d4149b5fb25
📒 Files selected for processing (7)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Architecture.hmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.hmlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Qubits.cpp
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
burgholzer
approved these changes
May 6, 2026
Member
There was a problem hiding this comment.
Finally found some time to get to this.
I pushed a couple of very minor changes that should hopefully not cause any problems.
This is definitely good to go from my side. I'll have you have a final look on the code. Feel free to merge whenever you are happy.
(Potentially request a final review from CodeRabbit)
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/include/mlir/Dialect/QCO/Transforms/Passes.td (1)
79-94: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePass description text still implies "multiple traversals" by default.
Lines 79–82 say the pass performs "multiple forward and backward traversals" to refine the mapping. With the new default
niterations=1, only a single forward traversal runs by default, which makes the descriptive text slightly misleading for the out-of-the-box behavior. Consider rewording (e.g., "may perform one or more forward/backward traversals, controlled byniterations").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td` around lines 79 - 94, Update the pass description paragraph to not imply multiple traversals by default: reword the sentences that state "the pass performs multiple forward and backward traversals..." to indicate the pass may perform one or more forward/backward traversals and that this behavior is controlled by the Option "niterations" (the Option<"niterations", ...> entry); ensure the text aligns with the default Option value niterations=1 (e.g., "the pass may perform one or more forward and backward traversals, controlled by `niterations`").
♻️ Duplicate comments (2)
mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.h (1)
33-39: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDoxygen for
createMappingPasscould document parameters.Past reviews flagged missing Doxygen on this declaration; the brief now exists, but
@param archand@param optionswould help callers understand:
- That
archis held viashared_ptr(presumably to allow safe sharing across pass clones during pipeline execution).- The semantics of
optionsand its default.📝 Proposed Doxygen enhancement
/** * `@brief` Create a mapping pass instance with the given target architecture. + * `@param` arch Target hardware architecture (shared so the same instance can + * be used across cloned pass instances). + * `@param` options Pass options; defaults to a default-constructed + * `@ref` MappingPassOptions. * `@returns` a pass object. */ std::unique_ptr<Pass> createMappingPass(std::shared_ptr<Architecture> arch, MappingPassOptions options = MappingPassOptions{});As per coding guidelines: "MUST use Doxygen-style comments in C++ code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.h` around lines 33 - 39, The declaration for createMappingPass lacks Doxygen `@param` tags; add brief `@param` entries describing that the arch parameter (std::shared_ptr<Architecture>) is stored/shared to allow safe reuse across pass clones and pipeline execution, and that options (MappingPassOptions) controls pass behavior with the default MappingPassOptions{} being used when omitted; reference the function name createMappingPass and the types Architecture and MappingPassOptions in the comments so callers understand ownership and default semantics.mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
354-362:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail fast when the architecture cannot host the function.
The TODO at Line 355 is still a real crash path: trial seeding and later placement assume every logical qubit has a hardware slot. With an empty or undersized architecture, this falls through to
Layout::getHardwareIndex()asserts instead of a clean pass failure.Suggested fix
for (auto func : getOperation().getOps<func::FuncOp>()) { - // TODO: Check if num-qubits > arch.nqubits + if (!arch) { + func.emitError() << "mapping pass requires a target architecture"; + signalPassFailure(); + return; + } + + const size_t requiredQubits = + range_size(func.getOps<AllocOp>()) + + range_size(func.getOps<ExtractOp>()); + if (requiredQubits > arch->nqubits()) { + func.emitError() << "mapping requires " << requiredQubits + << " qubits, but architecture only provides " + << arch->nqubits(); + signalPassFailure(); + return; + } // Create trials for initial layout refining. Currently, this includes // `ntrials` many random layouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp` around lines 354 - 362, Before seeding trials, check that the function's number of logical qubits does not exceed the hardware capacity and fail fast if it does: compute the function's logical qubit count (from the func::FuncOp being iterated), compare it to arch->nqubits(), and if logical_qubits > arch->nqubits() return a proper failure/emit an error from the pass instead of proceeding to create trials; this prevents later asserts in Layout::getHardwareIndex() when Layout::random(...) and placement assume a hardware slot exists for every logical qubit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Architecture.h`:
- Line 66: Document that neighboursOf(std::size_t u) returns a non-owning
ArrayRef view into the internal neighbours_ container and therefore its lifetime
is tied to the Architecture instance; add a brief Doxygen note on the
Architecture class or the neighboursOf method stating that neighbours_ is
populated once in the constructor, not resized thereafter, and the returned
ArrayRef remains valid only while the Architecture object remains alive and is
not moved or modified.
- Line 32: The public default constructor Architecture() makes the
getEmptyArchitecture() factory redundant and allows creation of an object that
will UB on neighboursOf(u)/distanceBetween(u,v)/areAdjacent(u,v) because
nqubits_/neighbours_/dist_/prev_ are empty; fix by making Architecture() private
(or protected) and converting getEmptyArchitecture() into the single static
factory (or declare it friend) so callers must use
Architecture::getEmptyArchitecture(); also update or add a brief comment on
nqubits()/name() vs. other methods documenting that default/empty architectures
only safely support trivial queries and other methods assume a populated
topology (or alternatively remove getEmptyArchitecture() and document that
Architecture() is intended for two-phase initialization and that callers must
not call neighboursOf/distanceBetween/areAdjacent on a zero-sized instance).
In `@mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.h`:
- Around line 27-31: The Doxygen for isExecutable(Region& region, const
Architecture& arch) should clarify that although the function accepts a
non-const Region& (per MLIR conventions), it does not mutate the region; update
the comment above the declaration of isExecutable to add a short note such as
"@note Does not mutate the Region" (or similar wording) so callers aren't
surprised, and leave the signature unchanged (reference the isExecutable
function and the Region/Architecture parameter names).
In `@mlir/include/mlir/Dialect/QCO/Utils/Qubits.h`:
- Around line 60-63: Update the Doxygen for the method getIndex in Qubits
(TypedValue<QubitType> getIndex) to document the precondition required by its
implementation: state that q must already have been added/registered (i.e.,
valueToIndex_.contains(q) must be true) before calling getIndex, for example
using an `@pre` clause like "q must have been previously added/registered and
exist in valueToIndex_"; refer to the getIndex declaration to locate where to
add this note.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 404-405: The swap statistic is being overwritten per function in
runOnOperation(): instead of assigning numSwaps = *res for each func.func,
accumulate across functions by adding the per-function result (i.e., numSwaps +=
*res) and ensure numSwaps is initialized to zero before the loop; update the
code paths that set numSwaps (and any related variables) so they sum results
from each function rather than replacing them.
---
Outside diff comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 79-94: Update the pass description paragraph to not imply multiple
traversals by default: reword the sentences that state "the pass performs
multiple forward and backward traversals..." to indicate the pass may perform
one or more forward/backward traversals and that this behavior is controlled by
the Option "niterations" (the Option<"niterations", ...> entry); ensure the text
aligns with the default Option value niterations=1 (e.g., "the pass may perform
one or more forward and backward traversals, controlled by `niterations`").
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.h`:
- Around line 33-39: The declaration for createMappingPass lacks Doxygen `@param`
tags; add brief `@param` entries describing that the arch parameter
(std::shared_ptr<Architecture>) is stored/shared to allow safe reuse across pass
clones and pipeline execution, and that options (MappingPassOptions) controls
pass behavior with the default MappingPassOptions{} being used when omitted;
reference the function name createMappingPass and the types Architecture and
MappingPassOptions in the comments so callers understand ownership and default
semantics.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 354-362: Before seeding trials, check that the function's number
of logical qubits does not exceed the hardware capacity and fail fast if it
does: compute the function's logical qubit count (from the func::FuncOp being
iterated), compare it to arch->nqubits(), and if logical_qubits >
arch->nqubits() return a proper failure/emit an error from the pass instead of
proceeding to create trials; this prevents later asserts in
Layout::getHardwareIndex() when Layout::random(...) and placement assume a
hardware slot exists for every logical qubit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c938d17-0e71-428b-8101-d53c94799369
📒 Files selected for processing (10)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Architecture.hmlir/include/mlir/Dialect/QCO/Transforms/Mapping/Mapping.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Dialect/QCO/Utils/Qubits.hmlir/lib/Dialect/QCO/Transforms/Mapping/Architecture.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Qubits.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
…ntum-toolkit/core into enh/improved-layering
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved and changes approved. |
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
3 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 updates the mapping pass in various ways:
walkProgramGraphdriverLayoutInfoand simply use the program-to-hardware vector as map key.routefunction for cold and hot routingChecklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.