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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

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


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

https://reviews.llvm.org/D130579