[Refactor] Enhance deterministic ordering in shared memory allocation merge.#1570
Conversation
… handling * Updated comparison logic in merge_shared_memory_allocations.cc to use name hints for deterministic ordering of variables instead of pointer comparisons. * Introduced a sorted vector of keys for shmem_allocs_ to ensure consistent iteration order when processing allocations. This refactor aims to improve the predictability of shared memory allocation handling in the transformation process.
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
📝 WalkthroughWalkthroughReplace pointer-based ordering with name-based ordering for shared memory allocations: introduce a sorted variable vector (by Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (1)
src/transform/merge_shared_memory_allocations.cc (1)
1104-1113: Good approach to ensure deterministic iteration.Creating a pre-sorted vector of variables sorted by name_hint ensures consistent processing order when building buffer info, which is essential for deterministic allocation offsets.
Optional: Consider adding a tiebreaker for rare edge cases
If two variables could ever have identical name_hints in practice, consider adding a secondary comparison criterion to guarantee total ordering:
std::sort(sorted_vars.begin(), sorted_vars.end(), [](const VarNode *a, const VarNode *b) { - return a->name_hint < b->name_hint; + if (a->name_hint != b->name_hint) { + return a->name_hint < b->name_hint; + } + // Fallback: compare type keys or other stable properties + return std::less<const VarNode*>()(a, b); });However, this is only necessary if name collisions are possible in your IR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/transform/merge_shared_memory_allocations.cc
🔇 Additional comments (3)
src/transform/merge_shared_memory_allocations.cc (3)
754-755: LGTM: Deterministic ordering achieved.The change from pointer-based to name-based comparison ensures consistent sorting across runs, addressing the PR's core objective.
1118-1118: Correct iteration over sorted collection.Iterating over
sorted_varsinstead of directly overshmem_allocs_ensures buffer info is built in a deterministic, name-based order.
1135-1135: Correct retrieval pattern for sorted iteration.Since the loop now iterates over
sorted_vars(just the keys), usingshmem_allocs_.at(var)correctly retrieves the correspondingAllocateNode.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This refactor aims to improve the predictability of shared memory allocation handling in the transformation process. Otherwise, a same input program may lead to different smem allocate result and may lead to nondeterministic kernel performance.
Before:
First Run:
Second Run:
After this pass, they'll be the same.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.