✨Arena-Based A* Search For Mapping Pass#1574
Merged
burgholzer merged 19 commits intomainfrom Mar 25, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors the A* SWAP search: Node stores a single swap + parent + depth (no per-node sequences), uses a bump-pointer arena and pointer-priority-queue, makes Layout usable as DenseMap key (LayoutInfo), reconstructs swap lists via parents, updates trial commit bookkeeping, and increases unit-test trials. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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}
Contributor
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 588-598: Constructing Node(child) invokes expensive heuristic work
(h()) before we check whether its layout is already in discovered; move the
discovered check to avoid wasted computation. Compute the swapped layout from
curr and swap (the same value used by Node::layout) and call
discovered.insert(swappedLayout) first; if insertion fails, skip creating Node;
otherwise construct Node(curr, swap, layers, arch, params) and emplace it into
frontier as before. Use the same layout representation Node uses so the
deduplication logic remains identical.
- Line 18: The code uses DenseMapInfo<SmallVector<IndexType>> (see uses of
getEmptyKey(), getTombstoneKey(), getHashValue()) but LLVM has no DenseMapInfo
specialization for SmallVector, causing compile failure; fix by either adding a
custom DenseMapInfo<SmallVector<IndexType>> specialization (implement
getEmptyKey, getTombstoneKey, getHashValue and equality) tailored for
SmallVector<IndexType>, or refactor the map/key usage in Mapping.cpp to use a
supported key type (e.g., convert keys to ArrayRef<IndexType>,
std::vector<IndexType>, or a packed primitive like std::pair/hashable struct)
and update the code paths that call getEmptyKey/getTombstoneKey/getHashValue
accordingly to use the supported DenseMapInfo specialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ab339b1-d359-42ac-8d81-1027f0f79838
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
560-596:⚠️ Potential issue | 🟠 MajorAvoid “first arrival wins” deduplication for
Layoutstates.
discoveredis updated when a child is generated, not when the cheapest path to that state is known. Hereh()depends only onlayout, whileg()depends onsequence.size(), so the sameLayoutcan be reached later with a shorter prefix. Pruning that later node can increase SWAP count and, with the fixed budget, make a solvable search returnfailure().♻️ One way to keep deduplication without losing better paths
- MinQueue frontier{}; - frontier.emplace(root); - DenseSet<Layout, LayoutInfo> discovered{root.layout}; + MinQueue frontier{}; + frontier.emplace(root); + DenseMap<Layout, std::size_t, LayoutInfo> bestDepth; + bestDepth.try_emplace(root.layout, 0); DenseSet<IndexGate> expansionSet; @@ - if (curr.isGoal(layers.front(), arch)) { + auto bestIt = bestDepth.find(curr.layout); + if (bestIt == bestDepth.end() || + curr.sequence.size() != bestIt->second) { + continue; + } + + if (curr.isGoal(layers.front(), arch)) { return curr.sequence; } @@ - // Multiple sequences of SWAPs may lead to the same layout. - // The if below ensures that we don't visit the same layout twice. - // TODO: In the future, should fidelities be ever considered, the - // sequence of SWAPs matters - so this will become more difficult. - if (!discovered.insert(child.layout).second) { + const auto depth = child.sequence.size(); + auto [it, inserted] = bestDepth.try_emplace(child.layout, depth); + if (!inserted && it->second <= depth) { continue; } + it->second = depth; frontier.emplace(std::move(child));🤖 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 560 - 596, The current deduplication uses DenseSet<Layout> discovered and inserts child.layout when generating a child, causing "first arrival wins" and potentially discarding a later better path because h() depends only on layout but g() (sequence.size()) varies. Replace discovered with a map from Layout to best-known g (e.g., DenseMap<Layout, std::size_t> or DenseMap<Layout,int>), and change the check in the child-generation block (inside the loop that constructs Node child and where discovered.insert(child.layout) is called) to: if layout not present OR child.sequence.size() < recorded_g then update the map with child.sequence.size() and push frontier.emplace(std::move(child)); otherwise skip pushing. Alternatively, defer marking a layout until node is popped and accepted (update the map there) so only popped nodes set the best g for that layout; apply this change to the discovered usage to avoid pruning better sequences for Layout while keeping deduplication.
🤖 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 533-535: The comment incorrectly describes the meaning of the b^3
budget; update the comment in Mapping.cpp where the "iteration budget is b^{3}"
is documented to state that b^3 bounds the number of node expansions (i.e.,
roughly exhausting all nodes up to depth 3, or exploring all paths of length up
to 3 in a b-ary search tree), rather than claiming it corresponds to exhausting
paths of length b^2; keep the note about the hard cap preventing impractical
runtimes and otherwise preserve the intent of the comment.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 560-596: The current deduplication uses DenseSet<Layout>
discovered and inserts child.layout when generating a child, causing "first
arrival wins" and potentially discarding a later better path because h() depends
only on layout but g() (sequence.size()) varies. Replace discovered with a map
from Layout to best-known g (e.g., DenseMap<Layout, std::size_t> or
DenseMap<Layout,int>), and change the check in the child-generation block
(inside the loop that constructs Node child and where
discovered.insert(child.layout) is called) to: if layout not present OR
child.sequence.size() < recorded_g then update the map with
child.sequence.size() and push frontier.emplace(std::move(child)); otherwise
skip pushing. Alternatively, defer marking a layout until node is popped and
accepted (update the map there) so only popped nodes set the best g for that
layout; apply this change to the discovered usage to avoid pruning better
sequences for Layout while keeping deduplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bf73a1e-19ae-4164-bcdb-d29bc20ed256
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
586-596: 🧹 Nitpick | 🔵 TrivialConsider checking
discoveredbefore constructing the fullNodeto avoid unnecessary heuristic computation.The
Nodeconstructor invokes the expensiveh()heuristic (iterating all layers/gates) before the layout is checked againstdiscovered. Computing the swapped layout first and checking membership would skip this cost for already-seen layouts.♻️ Proposed optimization
const IndexGate swap = std::minmax(hw0, hw1); if (!expansionSet.insert(swap).second) { continue; } - Node child(curr, swap, layers, arch, params); - - // Multiple sequences of SWAPs may lead to the same layout. - // The if below ensures that we don't visit the same layout twice. - // TODO: In the future, should fidelities be ever considered, the - // sequence of SWAPs matters - so this will become more difficult. - if (!discovered.insert(child.layout).second) { + // Check if layout was already discovered before constructing node. + Layout candidateLayout = curr.layout; + candidateLayout.swap(swap.first, swap.second); + if (!discovered.insert(candidateLayout).second) { continue; } - frontier.emplace(std::move(child)); + // Multiple sequences of SWAPs may lead to the same layout. + // The insert above ensures that we don't visit the same layout twice. + // TODO: In the future, should fidelities be ever considered, the + // sequence of SWAPs matters - so this will become more difficult. + frontier.emplace(curr, swap, layers, arch, params);🤖 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 586 - 596, Avoid constructing Node (which runs the expensive h() heuristic) before checking whether the resulting layout was already discovered: compute the layout that would result from applying swap to curr (the layout-only computation), check discovered.insert(newLayout).second and continue if false, and only then construct Node child(curr, swap, layers, arch, params) and frontier.emplace(std::move(child)); update references around Node, child.layout, discovered, swap and frontier.emplace accordingly.
🤖 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 239-242: The local 'using Info =
DenseMapInfo<decltype(a.programToHardware_)>;' in the static bool isEqual(const
Layout& a, const Layout& b) function redundantly re-declares/shadows the
class-level alias; remove that local using and simply reference the existing
Info alias when calling Info::isEqual(a.programToHardware_,
b.programToHardware_). Update the isEqual function in class Layout to use the
already-defined Info symbol (and ensure programToHardware_ is referenced with
the same names used there).
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 586-596: Avoid constructing Node (which runs the expensive h()
heuristic) before checking whether the resulting layout was already discovered:
compute the layout that would result from applying swap to curr (the layout-only
computation), check discovered.insert(newLayout).second and continue if false,
and only then construct Node child(curr, swap, layers, arch, params) and
frontier.emplace(std::move(child)); update references around Node, child.layout,
discovered, swap and frontier.emplace accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eff47478-c819-40aa-bb2d-475966051ad5
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
MatthiasReumann
added
c++
Contributor
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`:
- Line 563: The code currently uses a seen-set (DenseSet<Layout, LayoutInfo>
discovered{root.layout}) and marks a layout as discovered immediately, which
drops later arrivals that may have shorter prefixes (lower g()). Change the
discovered structure to record the best-known prefix length/cost per Layout
(e.g., store an integer prefixLen or numeric cost in LayoutInfo) and update
insertion/lookup logic in the expansion/marking code paths (the places that call
g() and currently insert into discovered) to: compute the new prefix cost with
g(), compare it to the stored best cost for that Layout, and only skip expansion
or overwrite the stored entry if the new cost is not an improvement; otherwise
accept and update the stored best cost and allow the node. Ensure all places
that currently insert or check discovered (the blocks around where discovered is
created and where layouts are marked) use this improved comparison.
- Around line 217-241: Add Doxygen-style comments (/** ... */) above the new
helper methods in class LayoutInfo: getEmptyKey(), getTombstoneKey(),
getHashValue(const Layout& l), and isEqual(const Layout& a, const Layout& b).
For getEmptyKey() and getTombstoneKey() add a brief description and an `@return`
documenting that a sentinel Layout is returned; for getHashValue() add a brief
description and an `@param` for "l" and an `@return` describing the returned
unsigned hash; for isEqual() add a brief description plus `@param` entries for "a"
and "b" and an `@return` indicating it returns true when equal. Use the project's
Doxygen comment style and place the comments immediately above the corresponding
method declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 030dcafe-0267-4509-ba87-edb0d9559780
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
MatthiasReumann
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (2)
216-244: 🛠️ Refactor suggestion | 🟠 MajorAdd method-level Doxygen to the new DenseMap hooks.
The class has a top-level block, but
getEmptyKey(),getTombstoneKey(),getHashValue(), andisEqual()still break the surrounding per-method documentation pattern. Please add brief@param/@returndocs for these helpers too.As per coding guidelines, "MUST use Doxygen-style comments in C++ code".
🤖 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 216 - 244, Add Doxygen-style method comments for the DenseMap hook helpers in class LayoutInfo: document getEmptyKey() and getTombstoneKey() with brief `@return` describing the special Layout returned, document getHashValue(const Layout& l) with `@param` describing the Layout input and `@return` describing the computed hash, and document isEqual(const Layout& a, const Layout& b) with `@param` entries for a and b and `@return` describing the equality result; place these method-level comments immediately above each corresponding method (getEmptyKey, getTombstoneKey, getHashValue, isEqual) following the existing per-method Doxygen style in the file.
577-579:⚠️ Potential issue | 🟠 MajorDeduplicate layouts before they enter the frontier.
The
bestDepthcheck only runs after pop, so the same layout is still enqueued and arena-allocated many times. A trivial backtrack already reproduces an old layout, and at Line 620 those stale pops also burn theb^3budget, so the search can fail long before it performsb^3real expansions.♻️ Proposed fix
llvm::SpecificBumpPtrAllocator<Node> arena; std::priority_queue<Node*, std::vector<Node*>, Node::ComparePointer> frontier; frontier.emplace(std::construct_at(arena.Allocate(), layout)); DenseMap<Layout, std::size_t, LayoutInfo> bestDepth; + bestDepth.try_emplace(layout, 0); DenseSet<IndexGate> expansionSet; std::size_t i = 0; while (!frontier.empty() && i < budget) { Node* curr = frontier.top(); frontier.pop(); - // Multiple sequences of SWAPs can lead to the same layout and the same - // layout creates the same child-nodes. Thus, if we've seen a layout - // already at a lower depth don't reexpand the current node (and hence - // recreate the same child nodes). - - const auto [it, inserted] = - bestDepth.try_emplace(curr->layout, curr->depth); - if (!inserted) { - const auto otherDepth = it->getSecond(); - if (curr->depth >= otherDepth) { - ++i; - continue; - } - - it->second = curr->depth; + if (curr->depth > bestDepth.lookup(curr->layout)) { + continue; } ... const IndexGate swap = std::minmax(hw0, hw1); if (!expansionSet.insert(swap).second) { continue; } + Layout childLayout = curr->layout; + childLayout.swap(swap.first, swap.second); + const auto childDepth = curr->depth + 1; + const auto [it, inserted] = + bestDepth.try_emplace(childLayout, childDepth); + if (!inserted && childDepth >= it->second) { + continue; + } + if (!inserted) { + it->second = childDepth; + } + frontier.emplace(std::construct_at(arena.Allocate(), curr, swap, layers, arch, params));Also applies to: 597-653
🤖 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 577 - 579, Duplicate layouts are being enqueued and arena-allocated repeatedly because the duplicate check happens after pop; modify the enqueue path for the frontier (where layouts are pushed into frontier/arena) to deduplicate before allocation by computing a canonical key for the layout and consulting a visited set/hash; only insert into arena and frontier if the key is not present, and add the key to visited at that time, leaving the existing bestDepth check on pop unchanged; reference the existing symbols: frontier, arena, bestDepth, pop, and the layout object/key computation when adding the duplicate-filtering check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 216-244: Add Doxygen-style method comments for the DenseMap hook
helpers in class LayoutInfo: document getEmptyKey() and getTombstoneKey() with
brief `@return` describing the special Layout returned, document
getHashValue(const Layout& l) with `@param` describing the Layout input and
`@return` describing the computed hash, and document isEqual(const Layout& a,
const Layout& b) with `@param` entries for a and b and `@return` describing the
equality result; place these method-level comments immediately above each
corresponding method (getEmptyKey, getTombstoneKey, getHashValue, isEqual)
following the existing per-method Doxygen style in the file.
- Around line 577-579: Duplicate layouts are being enqueued and arena-allocated
repeatedly because the duplicate check happens after pop; modify the enqueue
path for the frontier (where layouts are pushed into frontier/arena) to
deduplicate before allocation by computing a canonical key for the layout and
consulting a visited set/hash; only insert into arena and frontier if the key is
not present, and add the key to visited at that time, leaving the existing
bestDepth check on pop unchanged; reference the existing symbols: frontier,
arena, bestDepth, pop, and the layout object/key computation when adding the
duplicate-filtering check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad10f221-299b-41ef-8acc-1d30cb43662f
📒 Files selected for processing (2)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
219-244: 🧹 Nitpick | 🔵 TrivialAdd Doxygen comments to the
LayoutInfostatic methods.The methods
getEmptyKey(),getTombstoneKey(),getHashValue(), andisEqual()lack Doxygen-style documentation. Please add brief@return/@paramcomments consistent with the project's documentation standards.As per coding guidelines: "MUST use Doxygen-style comments in C++ code".
🤖 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 219 - 244, Add Doxygen-style comments for the static methods in class LayoutInfo: annotate getEmptyKey() and getTombstoneKey() with brief descriptions and `@return` documenting that they return a Layout representing the empty/tombstone hash-map sentinel; annotate getHashValue(const Layout& l) with a short summary and an `@param` describing 'l' and an `@return` explaining it yields the computed hash of the programToHardware_ map; annotate isEqual(const Layout& a, const Layout& b) with a summary plus `@param` entries for 'a' and 'b' and an `@return` describing it returns true when their programToHardware_ maps are equal. Ensure the comments use the project's Doxygen style and are placed immediately above each static method declaration in LayoutInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 219-244: Add Doxygen-style comments for the static methods in
class LayoutInfo: annotate getEmptyKey() and getTombstoneKey() with brief
descriptions and `@return` documenting that they return a Layout representing the
empty/tombstone hash-map sentinel; annotate getHashValue(const Layout& l) with a
short summary and an `@param` describing 'l' and an `@return` explaining it yields
the computed hash of the programToHardware_ map; annotate isEqual(const Layout&
a, const Layout& b) with a summary plus `@param` entries for 'a' and 'b' and an
`@return` describing it returns true when their programToHardware_ maps are equal.
Ensure the comments use the project's Doxygen style and are placed immediately
above each static method declaration in LayoutInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 952f6214-96bb-4f0c-bb29-ee36bc18e969
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
burgholzer
requested changes
Mar 24, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just some true nitpicks on some of the C++ details.
Otherwise, this looks great already!
Collaborator
Author
|
fyi: as it seems there are still SSA dominance issues. I've added a sortTopological for now. Let's maybe re-benchmark and then evaluate if implementing the anchoring strategy makes sense (I unfortunately think it will). |
Contributor
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 631-639: The inner loop in the block that builds the swap sequence
uses a variable named i that shadows an outer i declared earlier; rename the
inner loop counter to a distinct name (e.g., idx or j) to avoid shadowing and
improve readability: in the if (curr->isGoal(...)) branch where seq is sized
from curr->depth and you set std::size_t i = seq.size() - 1 and iterate for
(Node* n = curr; n->parent != nullptr; n = n->parent) { seq[i] = n->swap; --i;
}, change that inner i to a new identifier and update its uses (seq[...] and
--...) so Node*, curr, seq, depth and swap logic remain unchanged.
- Around line 632-637: The code initializes i as seq.size() - 1 which can
underflow when curr->depth == 0; change the initialization to avoid subtraction
on empty vector and guard the loop by computing i = seq.size(); if (i > 0) --i;
(or use a signed index like ssize_t start = static_cast<ssize_t>(seq.size()) - 1
and check start >= 0) before using seq[i] in the for loop that iterates Node* n
= curr; n->parent != nullptr; n = n->parent so that SmallVector<IndexGate> seq,
curr->depth, and the assignment seq[i] = n->swap are safe for depth==0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01a2771d-159d-445d-bd21-ec07373a04eb
📒 Files selected for processing (2)
mlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
Member
Interesting. Did this come up as part of this PR or in separate testing? |
Collaborator
Author
The last pull request (#1581) fixed an issue how we handled I will open another pull request after this one and rethink our current SWAP insertion strategy. There must be some kind of simple way to insert a SWAP and reorder its dependencies such that no SSA dominance violation occurs. There must be. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 649-661: Before allocating and constructing child nodes into the
arena (where frontier.emplace(std::construct_at(arena.Allocate(), curr, swap,
layers, arch, params)) is called), check the recorded bestDepth for the
candidate layout (use the same key you use later to look up bestDepth) and skip
constructing the node if bestDepth <= currentDepth+1; this avoids unnecessary
arena construction and heuristic h() computation for layouts already reached at
equal or better depth while keeping expansionSet and swap insertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32f5c084-28b6-41ec-b0bb-2c539231ad16
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
burgholzer
approved these changes
Mar 25, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
LGTM! Let's get this in now.
However, I also believe there must be some way to avoid these dominance issues without the "Keule" that is topological sorting.
This was referenced Mar 26, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request updates the A* search implementation of the mapping pass in two ways:
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.