[PATCH] D130579: AMDGPU: Use tablegen patterns for buffer global and flat atomic fadd

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

In D130579#3795182 <https://reviews.llvm.org/D130579#3795182>, @arsenm wrote:

> In D130579#3795146 <https://reviews.llvm.org/D130579#3795146>, @Petar.Avramovic wrote:
>
>> There are some conflicts with https://reviews.llvm.org/D130729, looks like an error to me.
>> Intrinsic patterns don't check address space (they will take any pointer)
>>
>>   define amdgpu_ps float @flat_atomic_fadd_f32_rtn_intrinsic(float addrspace(1)* %ptr, float %data) {
>>     %ret = call float @llvm.amdgcn.flat.atomic.fadd(float addrspace(1)* %ptr, float %data)
>>     ret float %ret
>>   }
>>   
>>   declare float @llvm.amdgcn.flat.atomic.fadd(float addrspace(1)*, float)
>>   
>>   
>>   define amdgpu_ps float @global_atomic_fadd_f32_rtn_intrinsic(float* %ptr, float %data) {
>>     %ret = call float @llvm.amdgcn.global.atomic.fadd(float* %ptr, float %data)
>>     ret float %ret
>>   }
>>   
>>   declare float @llvm.amdgcn.global.atomic.fadd(float*, float)
>>
>> tests like this will select %ptr ignoring it is from wrong address space. I would expect tests like this to fail to select.
>> Comments and behavior from D130729 <https://reviews.llvm.org/D130729> are based on translation of intrinsic to atomic rmw which are selected based on address space of  %ptr (removed in this patch, intrinsics have separate patterns).
>>
>> Planed fix:
>> intrinsic patterns should check address space, failing to select when pointer argument has wrong address space (I assume this is possible to do in tablegen).
>> D130729 <https://reviews.llvm.org/D130729> will also change intrinsic id, when it changes pointer.
>>
>> Any comments?
>
> The flat intrinsic with a global pointer is perfectly fine (and we recently started optimizing the address space for these in 20cf170e68def39dc50b59847afb8d9ab445703d <https://reviews.llvm.org/rG20cf170e68def39dc50b59847afb8d9ab445703d>). The global intrinsic with a flat pointer is more of a grey area and probably shouldn't select

Petar, just to explain, this is because the global addr space is a subset of the flat addr space, so any global pointer is also a valid flat pointer (but not vice versa). Given a global pointer it is OK to select a FLAT_ instruction, but it would still be better to select a GLOBAL_ instruction if one is available.


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

https://reviews.llvm.org/D130579