This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Use CAS loop for min/max atomics at system scope
ClosedPublic

Authored by doru1004 on Dec 7 2022, 9:11 AM.

Details

Summary

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.

Diff Detail

Event Timeline

doru1004 created this revision.Dec 7 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:11 AM
doru1004 requested review of this revision.Dec 7 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:11 AM
tianshilei1992 retitled this revision from [OpenMP][AMDGPU] Use CAS loop for min/max atomics at system scope to [AMDGPU] Use CAS loop for min/max atomics at system scope.
arsenm requested changes to this revision.Dec 7 2022, 9:27 AM

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
12905–12906

Should use AMDGPU::isFlatGlobalAddrSpace, in principal the same should apply to any other random address space number

12912–12915

move to predicate function to call at the use sites?

llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll
206

This breaks the purpose of this test. These should be updated to have a scope that doesn't require the cas loop

This revision now requires changes to proceed.Dec 7 2022, 9:27 AM
doru1004 added inline comments.Dec 7 2022, 9:38 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12905–12906

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?

12912–12915

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
206

I agree, this is a good point and I'll try to fix it.

arsenm added inline comments.Dec 7 2022, 9:41 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12905–12906

Yes

12912–12915

bool unsafeAtomicsDisabled(const Function &F) {

return ....

}

and call that at the use sites rather than eagerly parsing the attribute

arsenm added a comment.Dec 7 2022, 9:42 AM

I think you're best off replacing all changed tests with a specific scope, and introducing new tests specifically for system scoped atomics

doru1004 updated this revision to Diff 481100.Dec 7 2022, 4:21 PM
doru1004 marked 3 inline comments as done.Dec 7 2022, 4:25 PM

@arsenm I have updated the tests and made one of the two code changes you suggested. I have to do the function still.

arsenm added inline comments.Dec 7 2022, 4:26 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12987

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

doru1004 added inline comments.Dec 7 2022, 4:32 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12987

I have the same feeling.

arsenm added inline comments.Dec 7 2022, 4:48 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12987

Part of why I think we need D137361 is to get proper lowering for inc/dec for this

doru1004 updated this revision to Diff 481268.Dec 8 2022, 6:56 AM
doru1004 marked 2 inline comments as done.

I just updated the patch with the use of the function.

@arsenm please let me know if you have any further comments.

arsenm accepted this revision.Dec 8 2022, 4:00 PM

LGTM

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
12874

static and const&

This revision is now accepted and ready to land.Dec 8 2022, 4:00 PM
llvm/test/CodeGen/AMDGPU/global_atomics_i64.ll