[AMDGPU] Make sort ordering in AMDGPURewriteAGPRCopyMFMAImpl::eliminateSpillsOfReassignedVGPRs() strict weak.
#162493
Conversation
weiweichen
changed the title
AMDGPURewriteAGPRCopyMFMAImpl::eliminateSpillsOfReassignedVGPRs() strictly weak.AMDGPURewriteAGPRCopyMFMAImpl::eliminateSpillsOfReassignedVGPRs() strict weak.
weiweichen
requested review from
arsenm and
qcolombet
and removed request for
arsenm
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash in the AMDGPU compiler by making the sort comparator in eliminateSpillsOfReassignedVGPRs() satisfy strict weak ordering requirements. The original comparator violated the antisymmetry requirement needed by C++ STL sort functions.
- Fixed comparator logic to ensure strict weak ordering compliance
- Added additional conditions to the tie-breaker comparison
| return (A->reg().stackSlotIndex() < B->reg().stackSlotIndex()) && | ||
| ((A->weight() <= B->weight()) && (A->getSize() <= B->getSize())); |
There was a problem hiding this comment.
This comparator logic is incorrect and still violates strict weak ordering. The current implementation can return false for both comp(a,b) and comp(b,a) when elements have the same stackSlotIndex, which breaks antisymmetry. For strict weak ordering, when primary criteria are equal, you should use a single tie-breaker. Consider using: return A->reg().stackSlotIndex() < B->reg().stackSlotIndex(); as the sole comparator, or if multiple criteria are needed, use lexicographic ordering like: if (A->weight() != B->weight()) return A->weight() < B->weight(); if (A->getSize() != B->getSize()) return A->getSize() < B->getSize(); return A->reg().stackSlotIndex() < B->reg().stackSlotIndex();
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Consider using: return A->reg().stackSlotIndex() < B->reg().stackSlotIndex();
As mentioned in the issue, this is not the right solution for making strict weak ordering.
But it's also true that my change has the issue with when comp(a,b) and comp(b,a) can both be false 🤦 . Updating the logic to use the lexicographic ordering suggestion.
|
@llvm/pr-subscribers-backend-amdgpu Author: weiwei chen (weiweichen) ChangesTo fix the crash mentioned in
Make the comparator logic strict weak order. Full diff: https://github.com/llvm/llvm-project/pull/162493.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp b/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
index fedb694bfcc2a..caaa92999fd1a 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
@@ -490,7 +490,9 @@ void AMDGPURewriteAGPRCopyMFMAImpl::eliminateSpillsOfReassignedVGPRs() const {
return true;
// Tie breaker by number to avoid need for stable sort
- return A->reg().stackSlotIndex() < B->reg().stackSlotIndex();
+ // The ordering have to be strictly weak.
+ return (A->reg().stackSlotIndex() < B->reg().stackSlotIndex()) &&
+ ((A->weight() <= B->weight()) && (A->getSize() <= B->getSize()));
});
// FIXME: The APIs for dealing with the LiveInterval of a frame index are
|
|
|
||
| // Tie breaker by number to avoid need for stable sort | ||
| return A->reg().stackSlotIndex() < B->reg().stackSlotIndex(); | ||
| // The ordering have to be strictly weak. |
There was a problem hiding this comment.
| // The ordering have to be strictly weak. | |
| // The ordering has to be strictly weak. |
|
|
||
| // Tie breaker by number to avoid need for stable sort | ||
| return A->reg().stackSlotIndex() < B->reg().stackSlotIndex(); | ||
| return (A->reg().stackSlotIndex() < B->reg().stackSlotIndex()); |
There was a problem hiding this comment.
| return (A->reg().stackSlotIndex() < B->reg().stackSlotIndex()); | |
| return A->reg().stackSlotIndex() < B->reg().stackSlotIndex(); |
sure, any suggestion on how to add a test for this @arsenm ? 🙏 |
…e-162490/sort-strick-weak-order
|
Merge for now. Will add a test if @arsenm has suggestions on how to add test for this (sorry, I need some help with adding AMDGPU unit test) |
I'd probably just reduce from whatever you had. llvm-reduce the IR, then switch to llvm-reduce mir running the RA+post-RA pipeline. This case is probably quite difficult, but maybe using -stress-regalloc when reducing will help |
…ateSpillsOfReassignedVGPRs()` strict weak. (#162493) - [x] `sort` needs the comparator with strictly weak ordering, however current logic doesn't meet the [**Antisymmetry**](https://tanjim131.github.io/2020-05-22-strict-weak-ordering/#:~:text=Almost%20all%20C++%20STL%20containers,the%20person%20with%20greater%20height.) requirement with ``` sort 0x561ecd3d3db0,0x561eaba91d10 25 weight 0.000000e+00,0.000000e+00 size 650370,662754 slot 732,733 ``` Make the comparator logic strict weak order. Fixes #162490
sortneeds the comparator with strictly weak ordering, however current logic doesn't meet the Antisymmetry requirement withMake the comparator logic strict weak order.
Fixes #162490