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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/atomic_inc.ll:12
+
+  %ret = call i32 @llvm.amdgcn.atomic.inc.i32.p0i32(i32* %x, i32 %v,
+  i32 5, ; Ordering. AtomicOrdering.h: sequentially consistent
----------------
arsenm wrote:
> We should also have a builtin for this. It would not be difficult to add
Agreed and hopefully. It takes a lot of parameters so I was unsure about testing the codegen. Are you willing to review such a patch?


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/atomic_inc.ll:15
+  i32 2, ; Scope. SyncScope.h:  OpenCLAllSVMDevices is 2
+  i1 1 ; Volatile.  True for consistency with other atomic operations
+  )
----------------
arsenm wrote:
> This should be false. Where are you seeing this imply volatile? I think we started hacking around OpenCL volatile for atomics in device-libs
Fun story. We were using functions from hc_atomic.ll though a wrapper and those functions specified volatile. This does the same to preserve that behaviour.

However, dropping volatile from all of these functions doesn't actually break the build. Compiles about as cleanly as before, same set of tests pass. Therefore dropped the qualifier from all of these functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73076