✨ Skip Two-Qubit Blocks In Mapping Pass#1588
Conversation
MatthiasReumann
added
c++
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 5 seconds. ⌛ 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 (2)
📝 WalkthroughWalkthroughRefactors wire traversal and two-qubit-block advancement in the QCO mapping pass, changes trial layout generation to exclude the identity layout, updates a unit test to add additional gate sequence, and adds a PR link in CHANGELOG.md. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 391-396: ntrials is allowed to be 0 which leaves trials/results
empty and makes findBestTrial() return nullptr; change the logic in the trial
creation path (around the loop that fills trials using
Layout::random(arch.nqubits(), rng())) to explicitly handle this->ntrials == 0:
either reject the option up front (return an error) or ensure at least one
fallback trial is pushed (e.g., push an identity/initial layout before or
instead of random trials) so trials and results are never empty and
findBestTrial() cannot return nullptr; update any related uses that assume
trials.size() > 0 accordingly.
- Around line 643-649: After calling advanceUntilTwoQubitOp(first) and
advanceUntilTwoQubitOp(second) ensure you do not dereference exhausted
iterators: before using first.operation() or second.operation() check that
neither wire iterator is exhausted (e.g., test the iterator/handle sentinel or a
provided isDone()/isEnd()/hasValue() predicate) and break out if either is
exhausted; only call first.operation() and second.operation() when both are
valid. This prevents dereferencing a wire that hit the forward sentinel or
backward start inside advanceUntilTwoQubitOp.
🪄 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: f667debf-55c8-496f-be81-efe1a1317ac9
📒 Files selected for processing (3)
CHANGELOG.mdmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
393-397:⚠️ Potential issue | 🟠 MajorReject
ntrials == 0in code, not just in the help text.At Line 394,
0is still accepted. That leavestrialsandresultsempty, sofindBestTrial()returnsnullptrand the pass fails even on otherwise routable input.🛠️ Suggested guard
SmallVector<Layout> trials; + if (this->ntrials == 0) { + func.emitError() << "`ntrials` must be greater than 0"; + signalPassFailure(); + return; + } trials.reserve(this->ntrials); for (std::size_t i = 0; i < this->ntrials; ++i) { trials.emplace_back(Layout::random(arch.nqubits(), rng())); }🤖 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 393 - 397, Reject zero trials early: guard against this->ntrials == 0 in Mapping.cpp before reserving/emplacing into trials (the loop that calls Layout::random) and fail-fast (e.g., return error/emit diagnostic/throw) so trials and results are never left empty and findBestTrial() won't be called on an empty set; update the code path that constructs trials (referencing this->ntrials, trials, Layout::random, and findBestTrial()) to validate ntrials >= 1 and produce a clear diagnostic if it's 0.
🤖 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 67-69: Update the long pass description in Passes.td to match the
new behavior of ntrials: edit the textual paragraph that describes the pass (the
same block that contains the Option<"ntrials", "ntrials", "std::size_t", "4",
...>) to remove the claim that the pass "always explores the identity layout"
and instead state that the pass performs up to ntrials random forward/backward
mapping trials (possibly in parallel) to search for improved layouts, and
clarify that ntrials must be > 0; ensure the wording aligns with the
implementation in Mapping.cpp which no longer forces an identity-layout
exploration.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 610-619: Update the Doxygen for template <Direction d> static bool
proceedOnWire(const WireIterator& it) to correctly describe its contract: state
that it returns true when traversal can continue along the wire (i.e., for
Direction::Forward it returns true while the iterator has not reached the end
sentinel, and for Direction::Backward it returns true while the iterator's
operation is not an AllocOp), and false when traversal should stop; reference
the symbols proceedOnWire, Direction, WireIterator, and AllocOp in the comment
so callers understand the precise forward/backward semantics.
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 393-397: Reject zero trials early: guard against this->ntrials ==
0 in Mapping.cpp before reserving/emplacing into trials (the loop that calls
Layout::random) and fail-fast (e.g., return error/emit diagnostic/throw) so
trials and results are never left empty and findBestTrial() won't be called on
an empty set; update the code path that constructs trials (referencing
this->ntrials, trials, Layout::random, and findBestTrial()) to validate ntrials
>= 1 and produce a clear diagnostic if it's 0.
🪄 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: acee8b66-b0f5-4e2a-a57d-708a1dc6e920
📒 Files selected for processing (2)
mlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Already approved before. Still looks good. I am going to ignore the one duplicate comment from code rabbit about actually enforcing ntrials to be non-zero. One could place an assertion in the code for that. Feel free to merge if you are happy.
denialhaag
changed the title
2 participants
Description
This pull request re-implements the skip two-qubit block logic of the old implementation.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.