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&