This is an archive of the discontinued LLVM Phabricator instance.

[amdgpu] Atomic should be source of divergence.
ClosedPublic

Authored by hliao on Feb 24 2021, 8:13 AM.

Diff Detail

Event Timeline

hliao created this revision.Feb 24 2021, 8:13 AM
hliao requested review of this revision.Feb 24 2021, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 8:13 AM
hliao updated this revision to Diff 326100.Feb 24 2021, 8:15 AM

Update summary.

arsenm added inline comments.Feb 24 2021, 8:17 AM
llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll
28

Can you add exhaustive tests for all of these instructions (plus make sure llvm.amdgcn.atomic* work)

hliao updated this revision to Diff 326150.Feb 24 2021, 11:10 AM

Update tests with more atomic ops.

hliao marked an inline comment as done.Feb 24 2021, 11:10 AM
hliao added inline comments.
llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll
28

done, please check whether there are still missing ones.

hliao marked an inline comment as done.Feb 24 2021, 11:11 AM
kzhuravl added inline comments.Feb 24 2021, 11:13 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11893

always

arsenm accepted this revision.Feb 24 2021, 11:52 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11860

I didn't know about this one

This revision is now accepted and ready to land.Feb 24 2021, 11:52 AM
hliao marked an inline comment as done.Feb 24 2021, 12:06 PM
hliao added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11860

Only AArch64 uses that. Just add that conservatively.

This revision was landed with ongoing or failed builds.Feb 24 2021, 12:29 PM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Feb 25 2021, 5:30 AM
foad added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11854

Would it make sense to handle these generic nodes in a new TargetLowering::isSDNodeSourceOfDivergence so that we don't have to maintain this list in every target that cares about divergence?

arsenm added inline comments.Feb 25 2021, 5:33 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11854

We don't really have a generic definition of divergence. For CPU targets the answer would still be no

foad added inline comments.Feb 26 2021, 1:48 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11854

I was hoping CPU targets wouldn't care what we put in TargetLowering::isSDNodeSourceOfDivergence because they never consume the computed divergence information.

foad added inline comments.Feb 26 2021, 1:54 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11854

Another idea to avoid maintaining this list of opcodes: could you return true for any MemSDNode that isAtomic() ?

arsenm added inline comments.Feb 26 2021, 5:50 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11854

That should work