This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Skip buffer_wbl2 before atomic fence acquire
ClosedPublic

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

Details

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
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
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
This revision was automatically updated to reflect the committed changes.