⚡ Place SCF While Routing - #2016
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe mapping pass replaces recursive routing stack items with ChangesComposite routing refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
✨ 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 |
There was a problem hiding this comment.
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 winAdd a
Defaultbranch to both value-returningTypeSwitchuses indispatch. Both switches cover onlyscf::ForOp,scf::WhileOp,IfOp, andIndexSwitchOpand return a value.llvm::TypeSwitchholds the result in astd::optional. If no case matches, it asserts in debug builds and dereferences an emptystd::optionalin release builds, which is undefined behavior. The only guard is theassertat Line 1177 inadvance, which is removed underNDEBUG. The siblingplaceat Lines 1230-1234 already usesreport_fatal_errorin itsDefault.
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp#L1292-L1306: add aDefaultthat callsreport_fatal_errorwith the op name and returns an emptySmallVector<RoutingBundle, 0>.mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp#L1444-L1475: add aDefaultthat callsreport_fatal_errorwith the op name before theLayoutresult is produced.Based on learnings,
report_fatal_erroris the accepted idiom forDefaultdispatch invariants in QCO code undermlir/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
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
This pull request removes an additional traversal of the IR by placing SCF operations while routing when
Mode == RoutingMode::Hot. I've also renamedRecursiveRoutingStackItemtoCompositeUnitary.Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).