✨ Implement "vote-and-restore" strategy for qco.index_switch - #1951
Conversation
MatthiasReumann
added
c++
|
@coderabbitai full review |
|
✅ Action performedReview finished.
|
✅ Action performedFull review finished. |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesQCO mapping and layout
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MappingPass
participant IndexSwitchOp
participant RoutingBundles
participant Layout
MappingPass->>IndexSwitchOp: read tied operands and regions
MappingPass->>RoutingBundles: dispatch child routing bundles
RoutingBundles->>Layout: produce branch layouts
MappingPass->>Layout: vote for winning layout
MappingPass->>RoutingBundles: restore branches to winning layout
MappingPass->>IndexSwitchOp: realign yielded qubit values
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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: 3
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)
1458-1464: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment: the loop now realigns the
scf::ForOpterminator too.The note says the procedure is not required for
scf::ForOp, but the unified loop applies it to every region includingForOp's. Either drop the exception from the comment or skipForOpexplicitly.🤖 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 1458 - 1464, Update the comment above the loop iterating over op regions and children to remove the outdated exception claiming realignment is unnecessary for scf::ForOp, since the loop also processes its terminator; leave the unified loop behavior unchanged.
🤖 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 1271-1285: Add invariant Default branches to both TypeSwitches in
dispatch, including the children construction and exit handling switches. In
each Default branch, call the established report_fatal_error convention for
unsupported region-carrying operations instead of returning an empty vector or
default Layout; preserve the existing handling for all recognized operation
types.
In `@mlir/lib/Dialect/QCO/Utils/Layout.cpp`:
- Around line 30-34: Validate mapping in Layout::fromMapping before calling add:
ensure every hardware index is within the layout size and appears exactly once,
rejecting or asserting duplicates and out-of-range values. Preserve normal
construction for valid permutations and prevent add from receiving invalid
entries.
In `@mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp`:
- Around line 1190-1200: Update the caseBodies construction to store owning
callable objects whose lifetime extends through the qcoIndexSwitch invocation,
rather than non-owning function_refs to temporary lambdas. Capture the per-case
index by value so each callable retains its intended gate count, and remove the
stray semicolon after the construction loop.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 1458-1464: Update the comment above the loop iterating over op
regions and children to remove the outdated exception claiming realignment is
unnecessary for scf::ForOp, since the loop also processes its terminator; leave
the unified loop behavior unchanged.
🪄 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 Plus
Run ID: 27277a89-401d-4356-963a-a15d42a41c3b
📒 Files selected for processing (5)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Layout.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Layout.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Utils/Layout.h`:
- Around line 35-41: Make Layout(const size_t nqubits) private, placing it under
the class’s private section so callers cannot create zero-initialized
non-permutation layouts. Preserve access for the static fromMapping method,
which can use this constructor while constructing a valid mapping.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 1038-1049: Update the static vote function to validate that every
layout in the input range has the same qubit count as the first layout before
indexing scores or computing rank weights. Reject or assert mismatched layouts,
preserving the existing scoring behavior for uniformly sized layouts.
- Around line 1439-1453: Remove std::move from the returns of convergedLayout in
the IfOp case and winner in the IndexSwitchOp case. Both variables are const, so
moving falls back to copying; return them directly to allow normal copy elision
or implicit move behavior.
🪄 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 Plus
Run ID: 222b31f7-2730-42ae-965e-fae31462a532
📒 Files selected for processing (5)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Layout.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Layout.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
…quantum-toolkit/core into feat/map-index-switch-op
…quantum-toolkit/core into feat/map-index-switch-op
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp (2)
1147-1148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeed the generator deterministically for reproducible failures.
std::random_devicestill makes every CI run pick different per-case gate counts. Withntrials = 1, a routing failure becomes an unreproducible flake.🐛 Proposed fix
- std::random_device rd; - std::mt19937 gen(rd()); + std::mt19937 gen(42);🤖 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/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp` around lines 1147 - 1148, Replace the nondeterministic std::random_device seeding in the test’s generator setup with a fixed deterministic seed for std::mt19937, while preserving the existing per-case gate-count generation behavior so routing failures can be reproduced across runs.
1218-1220: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUnreachable
return to_vector(args);still present.Dead code after the unconditional
returnon the preceding statement, same as previously flagged.♻️ Proposed fix
return llvm::to_vector( builder.qcoIndexSwitch(index, args, cases, caseBodies, defaultCase)); - return to_vector(args); });🤖 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/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp` around lines 1218 - 1220, Remove the unreachable second return from the helper containing the builder.qcoIndexSwitch call. Keep the unconditional llvm::to_vector result return and delete the subsequent to_vector(args) statement.mlir/lib/Dialect/QCO/Utils/Layout.cpp (1)
30-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
fromMappingstill doesn't validate the mapping is a permutation.Same finding as a prior review of this function:
add()only bounds-checks, so duplicate hardware indices are silently accepted, leavingprogramToHardware_/hardwareToProgram_inconsistent. Today's two call sites (random,votein Mapping.cpp) happen to always pass valid permutations, but nothing enforces that for future callers.🐛 Proposed debug validation
Layout Layout::fromMapping(ArrayRef<size_t> mapping) { Layout layout(mapping.size()); + SmallVector<bool> seen(mapping.size(), false); for (const auto [prog, hw] : enumerate(mapping)) { + assert(hw < mapping.size() && !seen[hw] && + "mapping must be a permutation"); + seen[hw] = true; layout.add(prog, hw); } return layout; }🤖 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/Utils/Layout.cpp` around lines 30 - 36, Update Layout::fromMapping to validate that mapping is a permutation before or while populating the layout: reject duplicate hardware indices and any out-of-range values, rather than relying on add() alone. Preserve the existing construction behavior for valid mappings and ensure invalid input cannot leave programToHardware_ and hardwareToProgram_ inconsistent.
🤖 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/Utils/Layout.h`:
- Around line 78-86: Change the sized Layout constructor to private access
instead of protected. Keep it available to Layout’s static members fromMapping
and random, while preventing derived classes from constructing layouts with
uninitialized, non-permutation mappings and preserving the invariants used by
swap, restore, vote, and operator==.
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Utils/Layout.cpp`:
- Around line 30-36: Update Layout::fromMapping to validate that mapping is a
permutation before or while populating the layout: reject duplicate hardware
indices and any out-of-range values, rather than relying on add() alone.
Preserve the existing construction behavior for valid mappings and ensure
invalid input cannot leave programToHardware_ and hardwareToProgram_
inconsistent.
In `@mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp`:
- Around line 1147-1148: Replace the nondeterministic std::random_device seeding
in the test’s generator setup with a fixed deterministic seed for std::mt19937,
while preserving the existing per-case gate-count generation behavior so routing
failures can be reproduced across runs.
- Around line 1218-1220: Remove the unreachable second return from the helper
containing the builder.qcoIndexSwitch call. Keep the unconditional
llvm::to_vector result return and delete the subsequent to_vector(args)
statement.
🪄 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 Plus
Run ID: faffc7fb-bbaf-4938-b6fa-02a4990ddb36
📒 Files selected for processing (5)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Layout.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Layout.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
…quantum-toolkit/core into feat/map-index-switch-op
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This looks really clean!
I just have one minor observation where I'd like your input. Otherwise this can go in!
Reject duplicate and out-of-range hardware indices before constructing a layout and cover the public factory contract with focused tests. Assisted-by: GPT-5.6 via Codex
Assisted-by: GPT-5.6 via Codex
Assisted-by: GPT-5.6 via Codex
Assisted-by: GPT-5.6 via Codex
Assisted-by: GPT-5.6 via Codex
|
🤖 AI text below 🤖 Q-01: does the Borda vote have the intended semantics for
|
I am not really sure where this "cycling renaming" comes from, but here's how I see the problem: Each of the The goal is to find a "average" "winning" layout - or in other words - a program-to-hardware mapping. Finding this mapping should be somewhat efficient / fast. The Borda rank assigns points based on the position of the candidate (here: hardware index) on its ballot (here: program-to-hardware mapping). The program-qubit with index Thus, only the order of hardware indices determines the points. For the example given by Cody
the Borda Ranking is
The winner is thus Given that the program indices are ordered as discussed above, this is the correct choice. |
|
🤖 AI text below 🤖 I put the entire reasoning through another round of scutinizing and took some time to go through the reasoning, see below. To me the argumentation here makes sense and the proposed restauration strategy feels more natural and performant. What do you think? @MatthiasReumann Thanks for laying out your interpretation. I think there are two separate points here: one arithmetic issue in the example and one more fundamental question about what the aggregation should optimize. First, for the three ballots
I get the following totals:
Thus, candidates 0 and 2 tie with four points. Since the implementation breaks Borda ties by the lower hardware index, it returns The more fundamental question is whether program-qubit indices should serve as preference ranks in the first place. The implementation interprets a layout That is a valid Borda construction if ascending program index is intentionally an ordinal ranking. However, in the mapping pass, program indices are identities derived from extraction order. I do not see a reason why program qubit 0 should have greater importance than program qubit 1, or why the numeric distance between program indices should be meaningful for restoration. This is where the renaming argument comes from. If we consistently rename the logical qubits throughout the program and every branch layout, we have only renamed the tokens being routed. The hardware topology and the required physical movement have not changed. The restoration cost between two layouts is invariant under such a consistent renaming. Therefore, a convergence heuristic intended to minimize restoration overhead should ideally produce the correspondingly renamed result. This problem is not limited to Borda ties. For example, consider:
The Borda scores are Now consistently swap program-qubit identities 1 and 2. The equivalent layouts are:
Their scores are There are also counterexamples that do not rely on renaming at all. Consider three branch layouts on a four-qubit line:
Despite two of the three branches agreeing exactly on A, Borda produces scores Restoring all branches to A requires two adjacent SWAPs in total. Restoring them to the Borda result requires five. This is tie-free and shows that the Borda objective can disagree substantially with the restoration cost we ultimately care about. Given that minimizing inserted SWAPs and restoration overhead is the actual objective, would it make more sense to choose a topology-aware medoid among the branch layouts? Concretely, for every child layout
and select the child layout with the smallest total cost. The restoration sequences for the winning candidate could be cached and inserted directly, avoiding another round of computation. This has several properties that seem attractive here:
The selection requires quadratically many What do you think about replacing the Borda consensus with this branch-layout medoid? If compile time turns out to be a concern, we could compare the two strategies on representative switches before deciding, but the medoid seems more closely aligned with the optimization goal of the pass. |
|
Side note: The PR itself is obviously functionally correct and passes CI. So I am also happy to continue to merge this after resolving the conflicts and defer to a follow up. |
|
Many thanks! Besides me failing to add three numbers, I now know where this is coming from 👍🏻 I think the "topology-aware" argument is the strongest one here. I will try to find a solution for that. However, I am not sure if I am a fan of the medoid idea as this will add a significant overhead for many-case switches. Then, one could argue "just restore to the origin" is a better runtime / quality tradeoff. Otherwise, let's merge this for now to get #1867 going again. |
I agree, let's merge for now and improve in a follow-up! 👍🏻 I am not so sure if the overhead would really be that bad. But, to be honest, all of this is hypothetical. I have not yet really seen a use of the index switch in an algorithm, which would allow us to judge what to prioritize for performance. |
2 participants
Description
This pull request implements the "vote-and-restore" strategy for
qco.index_switchoperation. The final layout of each case region is considered as a "vote" of the respective case (the "voter") and the average (winning) layout is determined using the Borda Rank.Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).Assisted-by: [Model Name] via [Tool Name]footer.