Use CAS loop for min/max atomics at system scope. Currently a hardware atomic instruction is being used which does not work at system scope.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Most of these test changes break the purpose of the tests, which was proper legalization handling of the atomic instructions
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 12896–12897 | Should use AMDGPU::isFlatGlobalAddrSpace, in principal the same should apply to any other random address space number | |
| 12903–12906 | move to predicate function to call at the use sites? | |
| llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll | ||
| 184 | This breaks the purpose of this test. These should be updated to have a scope that doesn't require the cas loop | |
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 12896–12897 | This check here is just a factoring out of the switch statement since now this check is done in 2 places. Are you implying that this check was wrong or insufficient before? | |
| 12903–12906 | Could you please give me more details regarding this request? Again this is a check which was factored out of the switch statement because the same check is now done in two different locations. | |
| llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll | ||
| 184 | I agree, this is a good point and I'll try to fix it. | |
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 12896–12897 | Yes | |
| 12903–12906 | bool unsafeAtomicsDisabled(const Function &F) { return .... } and call that at the use sites rather than eagerly parsing the attribute | |
I think you're best off replacing all changed tests with a specific scope, and introducing new tests specifically for system scoped atomics
@arsenm I have updated the tests and made one of the two code changes you suggested. I have to do the function still.
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 12989 | I'm pretty sure all of the atomics (except maybe add?) have the same issue, but they don't all need to change at once | |
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 12989 | I have the same feeling. | |
static and const&