This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add support for amdgpu-unsafe-fp-atomics attribute
ClosedPublic

Authored by kzhuravl on Jan 25 2021, 1:47 PM.

Details

Summary

If amdgpu-unsafe-fp-atomics is specified, allow {flat|global}_atomic_add_f32 even if atomic modes don't match.

Diff Detail

Event Timeline

kzhuravl created this revision.Jan 25 2021, 1:47 PM
kzhuravl requested review of this revision.Jan 25 2021, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 1:47 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Jan 25 2021, 1:52 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11775–11777

Should sink this down to be the final check

AFAIR the idea was to always expand fp atomics without this unsafe option.

That is the safest thing to do. However, I'm now hearing some thinking that memory likely to be targeted by f.p. atomics is likely to be cached. But again, better to be safe at least until we're clear this will always be the case.

kzhuravl updated this revision to Diff 320924.Feb 2 2021, 3:10 PM
kzhuravl marked an inline comment as done.

Address review feedback.

arsenm added inline comments.Feb 2 2021, 3:13 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11775–11777

The idea was to hide the string attribute lookup as the final check. as in

!fpModeMatchesGlobalFPAtomicMode(RMW) && attributeStuff()

rampitec added inline comments.Feb 2 2021, 3:18 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11776–11778

I believe condition should be "or", not "and". Expand if either is not satisfied.

kzhuravl updated this revision to Diff 321239.Feb 3 2021, 2:56 PM
kzhuravl marked 2 inline comments as done.

Address review feedback.

rampitec accepted this revision.Feb 3 2021, 3:09 PM

LGTM

This revision is now accepted and ready to land.Feb 3 2021, 3:09 PM