[Openmp-commits] [PATCH] D73076: [libomptarget] Implement hip atomic functions in terms of intrinsics

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

In D73076#1832492 <https://reviews.llvm.org/D73076#1832492>, @jdoerfert wrote:

> Is is necessary, or useful, to have 3 different kinds of intrinsics here? We have `__atomic_fetch_max` and `__atomic_max_fetch` in clang, unsure if they could replace the OpenCL intrinsic. Similarly, there is `__atomic_compare_exchange` and `__atomic_exchange`. I would also be fine with only __amdgcn intrinsics or only __opencl ones but I am a little worried about mixing these 3 kinds.


I share this concern. I especially dislike the forced cast to _Atomic T* for the opencl intrinsics as that's skirting a bit close to an aliasing violation.

atomic_fetch_max I missed because gcc doesn't document it. In clang, it only compiles for 32 bit integer types (and we have a use of 64 bit in loop.cu). Some context in D46386 <https://reviews.llvm.org/D46386>.

I remember seeing different codegen for the opencl compare_exchange and the builtin but am not able to reproduce that now. There was some trial and error choosing intrinsics while comparing the codegen to hand written IR.

How about generic intrinsics where available:

- fetch_add
- exchange
- compare_exchange

Amdgcn prefixed IR functions for:

- atomic_inc
- atomic_fetch_max

with the intent to provide amdgcn intrinsics for inc and fetch_max?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73076