This is an archive of the discontinued LLVM Phabricator instance.

Differential D145524

[AMDGPU] Skip buffer_wbl2 before atomic fence acquire
ClosedPublic

Authored by rampitec on Mar 7 2023, 12:43 PM.

Details

Reviewers
t-tye
kzhuravl
Commits
rG59162e38590f: [AMDGPU] Skip buffer_wbl2 before atomic fence acquire
Summary

Memory models for gfx90a and gfx940 do not require buffer_wbl2
before the fence for acquire ordering, but we do insert the full
release.

Fixes: SWDEV-386785

Diff Detail

Event Timeline

rampitec created this revision.Mar 7 2023, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 12:43 PM
Herald added subscribers: kosarev, foad, kerbowa and 6 others. · View Herald Transcript
rampitec requested review of this revision.Mar 7 2023, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 12:43 PM
Herald added a subscriber: wdng. · View Herald Transcript
Harbormaster completed remote builds in B217935: Diff 503134.Mar 7 2023, 1:20 PM
t-tye requested changes to this revision.Mar 7 2023, 3:23 PM
t-tye added inline comments.
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
2212

Why is this waitcnt being added, I do not believe the memory model requires this. An acquire does not require previous memory operations to be complete. It only requires that the location being loaded has completed, followed by invalidating the caches consistent with the scope.

This revision now requires changes to proceed.Mar 7 2023, 3:23 PM
rampitec added inline comments.Mar 7 2023, 3:33 PM
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
2212

Memory model says this is the sequence:

fence        acquire      - agent        *none*     1. s_waitcnt lgkmcnt(0) &
                                                       vmcnt(0)

And then the same for the rest of the fence cases. The code produced looks consistent with the memory model to me.

t-tye accepted this revision.Mar 7 2023, 4:23 PM

After offline discussion, the extra waitcnt is needed because this is a fence, and an acquire needs a waitcnt to ensure a proceeding load atomic that pairs with the fence has completed before invalidating the cache. The memory model on AMDGPUUsage does show the extra waitcnt for the fence. Previously, the waitcnt was being generated as part of the release which is not required if the fence is just an acquire.

This revision is now accepted and ready to land.Mar 7 2023, 4:23 PM
rampitec marked an inline comment as done.Mar 7 2023, 4:55 PM
This revision was landed with ongoing or failed builds.Mar 8 2023, 1:24 AM
Closed by commit rG59162e38590f: [AMDGPU] Skip buffer_wbl2 before atomic fence acquire (authored by rampitec). · Explain Why
This revision was automatically updated to reflect the committed changes.
rampitec added a commit: rG59162e38590f: [AMDGPU] Skip buffer_wbl2 before atomic fence acquire.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
9 lines
test/
CodeGen/
AMDGPU/
12 lines

Diff 503134

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp

Show First 20 Lines • Show All 2,203 Lines • ▼ Show 20 Lines
bool SIMemoryLegalizer::expandAtomicFence(const SIMemOpInfo &MOI, bool SIMemoryLegalizer::expandAtomicFence(const SIMemOpInfo &MOI,
MachineBasicBlock::iterator &MI) { MachineBasicBlock::iterator &MI) {
assert(MI->getOpcode() == AMDGPU::ATOMIC_FENCE); assert(MI->getOpcode() == AMDGPU::ATOMIC_FENCE);
AtomicPseudoMIs.push_back(MI); AtomicPseudoMIs.push_back(MI);
bool Changed = false; bool Changed = false;
if (MOI.isAtomic()) { if (MOI.isAtomic()) {
if (MOI.getOrdering() == AtomicOrdering::Acquire || if (MOI.getOrdering() == AtomicOrdering::Acquire)
t-tyeUnsubmitted
Done
ReplyInline Actions

Why is this waitcnt being added, I do not believe the memory model requires this. An acquire does not require previous memory operations to be complete. It only requires that the location being loaded has completed, followed by invalidating the caches consistent with the scope.

t-tye: Why is this waitcnt being added, I do not believe the memory model requires this. An acquire…
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

Memory model says this is the sequence:

fence        acquire      - agent        *none*     1. s_waitcnt lgkmcnt(0) &
                                                       vmcnt(0)

And then the same for the rest of the fence cases. The code produced looks consistent with the memory model to me.

rampitec: Memory model says this is the sequence: ``` fence acquire - agent…
MOI.getOrdering() == AtomicOrdering::Release || Changed |= CC->insertWait(MI, MOI.getScope(), MOI.getOrderingAddrSpace(),
SIMemOp::LOAD | SIMemOp::STORE,
MOI.getIsCrossAddressSpaceOrdering(),
Position::BEFORE);
if (MOI.getOrdering() == AtomicOrdering::Release ||
MOI.getOrdering() == AtomicOrdering::AcquireRelease || MOI.getOrdering() == AtomicOrdering::AcquireRelease ||
MOI.getOrdering() == AtomicOrdering::SequentiallyConsistent) MOI.getOrdering() == AtomicOrdering::SequentiallyConsistent)
/// TODO: This relies on a barrier always generating a waitcnt /// TODO: This relies on a barrier always generating a waitcnt
/// for LDS to ensure it is not reordered with the completion of /// for LDS to ensure it is not reordered with the completion of
/// the proceeding LDS operations. If barrier had a memory /// the proceeding LDS operations. If barrier had a memory
/// ordering and memory scope, then library does not need to /// ordering and memory scope, then library does not need to
/// generate a fence. Could add support in this file for /// generate a fence. Could add support in this file for
/// barrier. SIInsertWaitcnt.cpp could then stop unconditionally /// barrier. SIInsertWaitcnt.cpp could then stop unconditionally
▲ Show 20 Lines • Show All 121 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/memory-legalizer-fence.ll

Show First 20 Lines • Show All 1,329 Lines • ▼ Show 20 Lines
; GFX90A-TGSPLIT-LABEL: agent_acquire_fence: ; GFX90A-TGSPLIT-LABEL: agent_acquire_fence:
; GFX90A-TGSPLIT: ; %bb.0: ; %entry ; GFX90A-TGSPLIT: ; %bb.0: ; %entry
; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-TGSPLIT-NEXT: s_endpgm ; GFX90A-TGSPLIT-NEXT: s_endpgm
; ;
; GFX940-NOTTGSPLIT-LABEL: agent_acquire_fence: ; GFX940-NOTTGSPLIT-LABEL: agent_acquire_fence:
; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX940-NOTTGSPLIT-NEXT: buffer_wbl2 sc1
; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc1 ; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc1
; GFX940-NOTTGSPLIT-NEXT: s_endpgm ; GFX940-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX940-TGSPLIT-LABEL: agent_acquire_fence: ; GFX940-TGSPLIT-LABEL: agent_acquire_fence:
; GFX940-TGSPLIT: ; %bb.0: ; %entry ; GFX940-TGSPLIT: ; %bb.0: ; %entry
; GFX940-TGSPLIT-NEXT: buffer_wbl2 sc1
; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX940-TGSPLIT-NEXT: buffer_inv sc1 ; GFX940-TGSPLIT-NEXT: buffer_inv sc1
; GFX940-TGSPLIT-NEXT: s_endpgm ; GFX940-TGSPLIT-NEXT: s_endpgm
; ;
; GFX11-WGP-LABEL: agent_acquire_fence: ; GFX11-WGP-LABEL: agent_acquire_fence:
; GFX11-WGP: ; %bb.0: ; %entry ; GFX11-WGP: ; %bb.0: ; %entry
; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0 ; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0
▲ Show 20 Lines • Show All 282 Lines • ▼ Show 20 Lines
; GFX90A-TGSPLIT-LABEL: agent_one_as_acquire_fence: ; GFX90A-TGSPLIT-LABEL: agent_one_as_acquire_fence:
; GFX90A-TGSPLIT: ; %bb.0: ; %entry ; GFX90A-TGSPLIT: ; %bb.0: ; %entry
; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-TGSPLIT-NEXT: s_endpgm ; GFX90A-TGSPLIT-NEXT: s_endpgm
; ;
; GFX940-NOTTGSPLIT-LABEL: agent_one_as_acquire_fence: ; GFX940-NOTTGSPLIT-LABEL: agent_one_as_acquire_fence:
; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX940-NOTTGSPLIT-NEXT: buffer_wbl2 sc1
; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc1 ; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc1
; GFX940-NOTTGSPLIT-NEXT: s_endpgm ; GFX940-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX940-TGSPLIT-LABEL: agent_one_as_acquire_fence: ; GFX940-TGSPLIT-LABEL: agent_one_as_acquire_fence:
; GFX940-TGSPLIT: ; %bb.0: ; %entry ; GFX940-TGSPLIT: ; %bb.0: ; %entry
; GFX940-TGSPLIT-NEXT: buffer_wbl2 sc1
; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX940-TGSPLIT-NEXT: buffer_inv sc1 ; GFX940-TGSPLIT-NEXT: buffer_inv sc1
; GFX940-TGSPLIT-NEXT: s_endpgm ; GFX940-TGSPLIT-NEXT: s_endpgm
; ;
; GFX11-WGP-LABEL: agent_one_as_acquire_fence: ; GFX11-WGP-LABEL: agent_one_as_acquire_fence:
; GFX11-WGP: ; %bb.0: ; %entry ; GFX11-WGP: ; %bb.0: ; %entry
; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) ; GFX11-WGP-NEXT: s_waitcnt vmcnt(0)
; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0 ; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0
▲ Show 20 Lines • Show All 270 Lines • ▼ Show 20 Lines
; ;
; SKIP-CACHE-INV-LABEL: system_acquire_fence: ; SKIP-CACHE-INV-LABEL: system_acquire_fence:
; SKIP-CACHE-INV: ; %bb.0: ; %entry ; SKIP-CACHE-INV: ; %bb.0: ; %entry
; SKIP-CACHE-INV-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; SKIP-CACHE-INV-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; SKIP-CACHE-INV-NEXT: s_endpgm ; SKIP-CACHE-INV-NEXT: s_endpgm
; ;
; GFX90A-NOTTGSPLIT-LABEL: system_acquire_fence: ; GFX90A-NOTTGSPLIT-LABEL: system_acquire_fence:
; GFX90A-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX90A-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX90A-NOTTGSPLIT-NEXT: buffer_wbl2
; GFX90A-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX90A-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX90A-NOTTGSPLIT-NEXT: buffer_invl2 ; GFX90A-NOTTGSPLIT-NEXT: buffer_invl2
; GFX90A-NOTTGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-NOTTGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-NOTTGSPLIT-NEXT: s_endpgm ; GFX90A-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX90A-TGSPLIT-LABEL: system_acquire_fence: ; GFX90A-TGSPLIT-LABEL: system_acquire_fence:
; GFX90A-TGSPLIT: ; %bb.0: ; %entry ; GFX90A-TGSPLIT: ; %bb.0: ; %entry
; GFX90A-TGSPLIT-NEXT: buffer_wbl2
; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX90A-TGSPLIT-NEXT: buffer_invl2 ; GFX90A-TGSPLIT-NEXT: buffer_invl2
; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-TGSPLIT-NEXT: s_endpgm ; GFX90A-TGSPLIT-NEXT: s_endpgm
; ;
; GFX940-NOTTGSPLIT-LABEL: system_acquire_fence: ; GFX940-NOTTGSPLIT-LABEL: system_acquire_fence:
; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX940-NOTTGSPLIT-NEXT: buffer_wbl2 sc0 sc1
; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc0 sc1 ; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc0 sc1
; GFX940-NOTTGSPLIT-NEXT: s_endpgm ; GFX940-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX940-TGSPLIT-LABEL: system_acquire_fence: ; GFX940-TGSPLIT-LABEL: system_acquire_fence:
; GFX940-TGSPLIT: ; %bb.0: ; %entry ; GFX940-TGSPLIT: ; %bb.0: ; %entry
; GFX940-TGSPLIT-NEXT: buffer_wbl2 sc0 sc1
; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX940-TGSPLIT-NEXT: buffer_inv sc0 sc1 ; GFX940-TGSPLIT-NEXT: buffer_inv sc0 sc1
; GFX940-TGSPLIT-NEXT: s_endpgm ; GFX940-TGSPLIT-NEXT: s_endpgm
; ;
; GFX11-WGP-LABEL: system_acquire_fence: ; GFX11-WGP-LABEL: system_acquire_fence:
; GFX11-WGP: ; %bb.0: ; %entry ; GFX11-WGP: ; %bb.0: ; %entry
; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) ; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0)
; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0 ; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0
▲ Show 20 Lines • Show All 280 Lines • ▼ Show 20 Lines
; ;
; SKIP-CACHE-INV-LABEL: system_one_as_acquire_fence: ; SKIP-CACHE-INV-LABEL: system_one_as_acquire_fence:
; SKIP-CACHE-INV: ; %bb.0: ; %entry ; SKIP-CACHE-INV: ; %bb.0: ; %entry
; SKIP-CACHE-INV-NEXT: s_waitcnt vmcnt(0) ; SKIP-CACHE-INV-NEXT: s_waitcnt vmcnt(0)
; SKIP-CACHE-INV-NEXT: s_endpgm ; SKIP-CACHE-INV-NEXT: s_endpgm
; ;
; GFX90A-NOTTGSPLIT-LABEL: system_one_as_acquire_fence: ; GFX90A-NOTTGSPLIT-LABEL: system_one_as_acquire_fence:
; GFX90A-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX90A-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX90A-NOTTGSPLIT-NEXT: buffer_wbl2
; GFX90A-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX90A-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX90A-NOTTGSPLIT-NEXT: buffer_invl2 ; GFX90A-NOTTGSPLIT-NEXT: buffer_invl2
; GFX90A-NOTTGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-NOTTGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-NOTTGSPLIT-NEXT: s_endpgm ; GFX90A-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX90A-TGSPLIT-LABEL: system_one_as_acquire_fence: ; GFX90A-TGSPLIT-LABEL: system_one_as_acquire_fence:
; GFX90A-TGSPLIT: ; %bb.0: ; %entry ; GFX90A-TGSPLIT: ; %bb.0: ; %entry
; GFX90A-TGSPLIT-NEXT: buffer_wbl2
; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX90A-TGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX90A-TGSPLIT-NEXT: buffer_invl2 ; GFX90A-TGSPLIT-NEXT: buffer_invl2
; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol ; GFX90A-TGSPLIT-NEXT: buffer_wbinvl1_vol
; GFX90A-TGSPLIT-NEXT: s_endpgm ; GFX90A-TGSPLIT-NEXT: s_endpgm
; ;
; GFX940-NOTTGSPLIT-LABEL: system_one_as_acquire_fence: ; GFX940-NOTTGSPLIT-LABEL: system_one_as_acquire_fence:
; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry ; GFX940-NOTTGSPLIT: ; %bb.0: ; %entry
; GFX940-NOTTGSPLIT-NEXT: buffer_wbl2 sc0 sc1
; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX940-NOTTGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc0 sc1 ; GFX940-NOTTGSPLIT-NEXT: buffer_inv sc0 sc1
; GFX940-NOTTGSPLIT-NEXT: s_endpgm ; GFX940-NOTTGSPLIT-NEXT: s_endpgm
; ;
; GFX940-TGSPLIT-LABEL: system_one_as_acquire_fence: ; GFX940-TGSPLIT-LABEL: system_one_as_acquire_fence:
; GFX940-TGSPLIT: ; %bb.0: ; %entry ; GFX940-TGSPLIT: ; %bb.0: ; %entry
; GFX940-TGSPLIT-NEXT: buffer_wbl2 sc0 sc1
; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0) ; GFX940-TGSPLIT-NEXT: s_waitcnt vmcnt(0)
; GFX940-TGSPLIT-NEXT: buffer_inv sc0 sc1 ; GFX940-TGSPLIT-NEXT: buffer_inv sc0 sc1
; GFX940-TGSPLIT-NEXT: s_endpgm ; GFX940-TGSPLIT-NEXT: s_endpgm
; ;
; GFX11-WGP-LABEL: system_one_as_acquire_fence: ; GFX11-WGP-LABEL: system_one_as_acquire_fence:
; GFX11-WGP: ; %bb.0: ; %entry ; GFX11-WGP: ; %bb.0: ; %entry
; GFX11-WGP-NEXT: s_waitcnt vmcnt(0) ; GFX11-WGP-NEXT: s_waitcnt vmcnt(0)
; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0 ; GFX11-WGP-NEXT: s_waitcnt_vscnt null, 0x0
▲ Show 20 Lines • Show All 252 Lines • Show Last 20 Lines