[PATCH] D96639: [AMDGPU] Add two TSFlags: IsAtomicNoRtn and IsAtomicRtn

[Tony Tye via Phabricator via llvm-commits]

t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.

In D96639#2561302 <https://reviews.llvm.org/D96639#2561302>, @rampitec wrote:

> In D96639#2561212 <https://reviews.llvm.org/D96639#2561212>, @rampitec wrote:
>
>> @t-tye memory legalizer changes are due to the fact that SIMemoryLegalizer::isAtomicRet() was returning false for the ds_wrxchg_rtn_b32.
>>
>> Please verify if the tests now look correctly or not. I am checking memory model for GFX10 and I do not see vmcnt required for these situations, but I do not see vscnt which was generated before there as well. Looks like this has uncovered a bug in either legalizer (likely) or memory model description. On top of that I do not see memory model describing atomicrmw acquire agent local combination.
>
> I.e. I understand why vscnt is replaced with vmcnt, it was treated as store and now as load. This is fine. I don't understand what does it do with vmem at all.

The test result changes look correct. The legalizer generated code was not incorrect before, it was just not optimal. The generation of the vmcnt or vscnt is not required at all as the ds_wrxchg_rtn instruction only acts on the local memory which is only accessibly up to workgroup scope. Accessing at a wider scope "decays" to workgroup scope. It would be good to consider updating the memory legalizer to exploit this fact.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96639/new/

https://reviews.llvm.org/D96639