Skip to content

⚡ Place SCF While Routing - #2016

Open
MatthiasReumann wants to merge 7 commits into
mainfrom
enh/inline-placement-scf
Open

⚡ Place SCF While Routing#2016
MatthiasReumann wants to merge 7 commits into
mainfrom
enh/inline-placement-scf

Conversation

@MatthiasReumann

@MatthiasReumann MatthiasReumann commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Description

This pull request removes an additional traversal of the IR by placing SCF operations while routing when Mode == RoutingMode::Hot. I've also renamed RecursiveRoutingStackItem to CompositeUnitary.

Checklist

  • The pull request only contains commits that are focused and relevant to this change.
  • I have added appropriate tests that cover the new/changed functionality.
  • I have updated the documentation to reflect these changes.
  • I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
  • I have added migration instructions to the upgrade guide (if needed).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

If PR contains AI-assisted content:

  • Any agent that created, edited, or submitted GitHub content was explicitly authorized for that scope, as required by our AI Usage Guidelines.
  • Every agent-authored or agent-edited public text body begins with the visible disclosure 🤖 *AI text below* 🤖 (titles are exempt).
  • I have disclosed AI assistance in the PR description.
  • I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it.

@MatthiasReumann MatthiasReumann self-assigned this Aug 7, 2026
@MatthiasReumann MatthiasReumann added c++ Anything related to C++ code MLIR Anything related to MLIR labels Aug 7, 2026
@MatthiasReumann MatthiasReumann added this to the MLIR Support milestone Aug 7, 2026
@MatthiasReumann

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Improved routing for composite control-flow operations and their child regions.
    • Added support for selectively updating wire layouts and mappings during routing.
    • Enhanced reconciliation of routing results across control-flow paths.
  • Bug Fixes

    • Improved propagation and restoration of qubit mappings during complex routing scenarios.
    • Added safer handling for retrieving qubit values from operation results.

Walkthrough

The mapping pass replaces recursive routing stack items with CompositeUnitary records. It adds selective RoutingBundle::Patch updates, routes child regions independently, merges statistics, and propagates layouts and wire mappings through the main route loop.

Changes

Composite routing refactor

Layer / File(s) Summary
Routing contracts and composite placement
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
The pass introduces CompositeUnitary, RoutingBundle::Patch, result-based qubit lookup, and non-recursive composite placement for supported control-flow operations.
Child-region routing and statistics
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Composite children are dispatched independently. Returned patches and statistics are propagated. Reconciliation SWAP operations contribute to accumulated statistics.
Route-loop patch integration
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
The route loop applies patches, advances composite wires, returns FailureOr<Statistics>, and uses independent statistics for refinement trials.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested labels: enhancement, c++, MLIR

Suggested reviewers: burgholzer

Poem

A rabbit maps each winding wire,
Bundles patches, then climbs higher.
Child paths route through loops anew,
Statistics hop from queue to queue.
Layouts converge beneath moonlight bright.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description has the required summary and checklist, but it lacks the visible AI disclosure and incorrectly marks changelog updates as complete. Add the required visible AI disclosure and either update the changelog or uncheck the changelog checklist item.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: placing SCF While operations during routing.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch enh/inline-placement-scf

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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)

1292-1306: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add a Default branch to both value-returning TypeSwitch uses in dispatch. Both switches cover only scf::ForOp, scf::WhileOp, IfOp, and IndexSwitchOp and return a value. llvm::TypeSwitch holds the result in a std::optional. If no case matches, it asserts in debug builds and dereferences an empty std::optional in release builds, which is undefined behavior. The only guard is the assert at Line 1177 in advance, which is removed under NDEBUG. The sibling place at Lines 1230-1234 already uses report_fatal_error in its Default.

  • mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp#L1292-L1306: add a Default that calls report_fatal_error with the op name and returns an empty SmallVector<RoutingBundle, 0>.
  • mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp#L1444-L1475: add a Default that calls report_fatal_error with the op name before the Layout result is produced.

Based on learnings, report_fatal_error is the accepted idiom for Default dispatch invariants in QCO code under mlir/lib/Dialect/QCO/.

🤖 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 1292 -
1306, Add Default handlers to both value-returning TypeSwitch uses in dispatch:
at mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp:1292-1306, call
report_fatal_error with the operation name and return an empty
SmallVector<RoutingBundle, 0>; at
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp:1444-1475, call
report_fatal_error with the operation name before producing the Layout result.
Use the existing place dispatch pattern as guidance.

Source: Learnings

🤖 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`:
- Line 1289: Update the structured binding for composite in the surrounding
mapping function to bind by const reference instead of copying it, preserving
read-only access to the SmallVector<size_t> indices member.
- Around line 1197-1253: Add a boundary guard in place before calling std::prev
on each excluded parent.wires[i]. Detect wires still at their source/first
operation and use the appropriate valid iterator or value instead of
decrementing past the beginning; preserve the existing predecessor selection for
wires with a valid prior operation.

---

Outside diff comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 1292-1306: Add Default handlers to both value-returning TypeSwitch
uses in dispatch: at
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp:1292-1306, call
report_fatal_error with the operation name and return an empty
SmallVector<RoutingBundle, 0>; at
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp:1444-1475, call
report_fatal_error with the operation name before producing the Layout result.
Use the existing place dispatch pattern as guidance.
🪄 Autofix

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: d4f202a8-b01b-47a9-826c-dae6a850dd68

📥 Commits

Reviewing files that changed from the base of the PR and between 52cf1e7 and 4070848.

📒 Files selected for processing (1)
  • mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp

Comment thread mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Comment thread mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp Outdated
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.73913% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...lir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp 96.7% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@MatthiasReumann
MatthiasReumann marked this pull request as ready for review August 7, 2026 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Anything related to C++ code MLIR Anything related to MLIR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant