✨ Map qco.if statements by "converge" strategy - #1870
Conversation
…antum-toolkit/core into enh/sc-map-scf-if-stmt
MatthiasReumann
added
c++
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR extends the QCO mapping/routing pass to support ChangesIfOp Mapping/Routing Support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
🧹 Nitpick comments (2)
mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp (1)
691-691: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out debug call.
// entry->dumpPretty();is a leftover debug artifact.🧹 Suggested cleanup
- // entry->dumpPretty(); -🤖 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` at line 691, Remove the leftover debug artifact in the test around the entry handling code by deleting the commented-out entry->dumpPretty() call. Keep the surrounding test logic intact and make sure no other debug-only commented statements remain in this area.mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
1120-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInner loop variable
ishadows the outerfor (size_t i : indices).The nested
for (size_t i = 0; i < children.size(); ++i)in both the forward and backward branches shadows the outer range variable. It happens to be harmless because the outer loop is range-based, but it obscures whichiis in scope (and will trip-Wshadow). Consider renaming the inner index.♻️ Suggested rename
- if constexpr (Direction == WireDirection::Forward) { - for (size_t i = 0; i < children.size(); ++i) { - children[i].wires.emplace_back(args[i]); - children[i].infos.map(index, prog); - } - } else { + if constexpr (Direction == WireDirection::Forward) { + for (size_t c = 0; c < children.size(); ++c) { + children[c].wires.emplace_back(args[c]); + children[c].infos.map(index, prog); + } + } else { const std::array yields{ ifOp.getTiedThenYieldedValue(args[0])->get(), ifOp.getTiedElseYieldedValue(args[1])->get()}; - for (size_t i = 0; i < children.size(); ++i) { - children[i].wires.emplace_back(yields[i]); - children[i].infos.map(index, prog); - } + for (size_t c = 0; c < children.size(); ++c) { + children[c].wires.emplace_back(yields[c]); + children[c].infos.map(index, prog); + } }🤖 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 1120 - 1133, The nested index variable inside the `Direction` branches in `Mapping.cpp` shadows the outer `for (size_t i : indices)` loop variable, which can trigger shadowing warnings and makes the scope unclear. Rename the inner loop index in both branches of the `if constexpr (Direction == WireDirection::Forward)` block to a distinct name, and update the corresponding `children[...]` accesses consistently. Keep the change localized to the loop that populates `children[i].wires` and `children[i].infos.map(index, prog)` so the outer `indices` loop remains unaffected.
🤖 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.
Nitpick comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 1120-1133: The nested index variable inside the `Direction`
branches in `Mapping.cpp` shadows the outer `for (size_t i : indices)` loop
variable, which can trigger shadowing warnings and makes the scope unclear.
Rename the inner loop index in both branches of the `if constexpr (Direction ==
WireDirection::Forward)` block to a distinct name, and update the corresponding
`children[...]` accesses consistently. Keep the change localized to the loop
that populates `children[i].wires` and `children[i].infos.map(index, prog)` so
the outer `indices` loop remains unaffected.
In `@mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp`:
- Line 691: Remove the leftover debug artifact in the test around the entry
handling code by deleting the commented-out entry->dumpPretty() call. Keep the
surrounding test logic intact and make sure no other debug-only commented
statements remain in this area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4f6c932-6662-4390-94eb-dd2943906e53
📒 Files selected for processing (5)
CHANGELOG.mdmlir/lib/Dialect/QCO/IR/SCF/IfOp.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Graph.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
…g pass • Addons Construction: Refined the filtering logic in scf::ForOp and IfOp to use SmallPtrSet and llvm::make_filter_range. This achieves $O(N+M)$ complexity, efficiently filtering qubit inputs and eliminating expensive intermediate DenseSet creation and repeated is_contained checks. • WireInfos Mapping: Replaced DenseMap<size_t, size_t> with std::vector<size_t> for wire-to-program index lookups. This provides $O(1)$ direct access and improves cache locality, which is critical for scalability in larger circuits. • A Search Optimization: Replaced DenseSet<IndexPairType> with SmallVector<IndexPairType, 8> in the A search expansion set. This leverages the small, constant architecture connectivity to avoid unnecessary hashing and heap allocations in the hot search loop.
There was a problem hiding this comment.
Spent way too much time with cc63e32 now probably 😅
Added an AI-summary to the commit message as well with some more details. Overall, this should improve the efficiency of quite a few of the code pieces in the mapping pass.
One of the main arguments was that DenseSet is pretty heavy when the number of elements is small and a simple linear search over a small vector is typically faster and more cache friendly.
Similarly, I think it is safe to assume that we can contiguously index the program and hardware qubits (even though the hardware identifiers may not be contiguous for some architectures). I haven't really come across relevant modern architectures that do not index their hardware qubits from 0...N-1. Hence, the program to hardware mapping can be realized with SmallVectors instead of DenseSets as well.
I also optimized some of the SCF tracking code to compute fewer temporaries.
Let me know how you feel about the changes. I am happy with this as it is now 🙌🏼
Edit: I'll leave adding the 5 [[nodiscard]s to you 😌
|
Many thanks! Much appreciated 🙏 Mapping-code grows larger and larger; Therefore, four eyes are definitely better than two! Otherwise, I am happy with the changes you made. Feel free to press the merge button whenever you see this comment 👀 |
Alright, then let's not hold that off for too long and get it in. |
2 participants
Description
dispatchmethod to handle the recursiveroutecalling and insertion of epilogue SWAPs.FGraphdatastructure to reduce code duplicationChecklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.Summary by CodeRabbit
New Features
Bug Fixes
Tests