[libomptarget] Implement most hip atomic functions in terms of intrinsics

[JonChesterfield]

Summary

[libomptarget] Implement hip atomic functions in terms of intrinsics

All but atomicInc can be implemented using type generic clang intrinsics.
There is not yet a corresponding intrinsic for atomicInc in clang, only one in
LLVM. This patch leaves atomicInc as an unresolved symbol.

[jdoerfert]

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.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt

73 ↗

(On Diff #239220)

Add a TODO here and in the ll file to state this is a *temporary workaround*. Also state the proper solution please.

[JonChesterfield]

In 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.

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?

[JonChesterfield]

• Prefer non-opencl atomics where available, comment the IR function

[jdoerfert]

I like this better. Interestingly, we could even think about sharing the clang builtin versions across targets. I'm fine with us keeping it this way for now. What do u think?

openmp/libomptarget/deviceRTLs/amdgcn/src/hip_atomics.h

53

Add a TODO where we want different builtins later on.

[arsenm]

openmp/libomptarget/deviceRTLs/amdgcn/src/atomic_inc.ll

12 ↗

(On Diff #239446)

We should also have a builtin for this. It would not be difficult to add

15 ↗

(On Diff #239446)

This should be false. Where are you seeing this imply volatile? I think we started hacking around OpenCL volatile for atomics in device-libs

[JonChesterfield]

• review comments

[JonChesterfield]

• comment

[JonChesterfield]

openmp/libomptarget/deviceRTLs/amdgcn/src/atomic_inc.ll

12 ↗

(On Diff #239446)

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?

15 ↗

(On Diff #239446)

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.

[arsenm]

openmp/libomptarget/deviceRTLs/amdgcn/src/amdgcn_atomic.ll

20 ↗

(On Diff #239476)

There must be an existing builtin for this?

[JonChesterfield]

openmp/libomptarget/deviceRTLs/common/src/loop.cu

796 ↗

(On Diff #239476)

Buffer and elem are already uint64_t, and unsigned long long == 8 bytes on nvptx and amdgcn, so I'm not sure why there was a cast here.

[JonChesterfield]

openmp/libomptarget/deviceRTLs/amdgcn/src/amdgcn_atomic.ll

20 ↗

(On Diff #239476)

There is an opencl builtin, but it wants the pointer type cast to an atomic. I'm reluctant to use that when the underlying type is not atomic qualified. Used in the previous version of this diff.

There is also a clang builtin, but only for 32 bits.

[JonChesterfield]

Discussed in the biweekly openmp call. Agreed that we should write the clang intrinsics first as deviceRTL isn't on the critical path for offloading to amdgcn anyway. I'll update this patch once they're available.

[JonChesterfield]

• Use atomic max, reformat

[JonChesterfield]

Atomic max is implemented on trunk, as of D55562. Thanks Tim! I didn't notice it land. And failed to spot the difference between fetch_max and max_fetch.

Generates the same IR as the previous revision, updated diff accordingly.

[JonChesterfield]

• Checked cuda documentation, replace max_fetch with fetch_max to match their semantics

[JonChesterfield]

I have an amdgcn intrinsic for atomic_inc working without any semantic checks. SemaChecking is rather complicated though so it's going to take a little while to work out what changes need to go there.

[JonChesterfield]

• Simplify, drop atomicInc impl

[JonChesterfield]

At some point in the future we may want to change over to using atomic functions that closer match the C++ ones, and let nvptx implement those in terms of cuda.

This patch is now very minimal - map the few cuda style atomic functions used onto clang's intrinsics, where available.

[JonChesterfield]

clang-tidy failed because it tried to compile this file for x86, which doesn't work, and because it doesn't like the naming conventions. I can do T r; => T R; and capitalise various names if we like, but the unknown attributes and error on looking for -DAMDGCN will need an infrastructure change to resolve.

[jdoerfert]

I'm fine with this, @arsenm ?

[JonChesterfield]

Only one of the atomicInc functions is used and clang (trunk) now has an intrinsic that can be used to implement it.

openmp/libomptarget/deviceRTLs/amdgcn/src/hip_atomics.h

 //===---- hip_atomics.h - Declarations of hip atomic functions ---- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef OMPTARGET_AMDGCN_HIP_ATOMICS_H
 #define OMPTARGET_AMDGCN_HIP_ATOMICS_H
 
 #include "target_impl.h"
 
-DEVICE unsigned atomicAdd(unsigned *address, unsigned val);
-DEVICE int atomicAdd(int *address, int val);
-DEVICE unsigned long long atomicAdd(unsigned long long *address,
-                                    unsigned long long val);
-
+// inc requires an amdgcn specific intrinsic which is not yet available
 DEVICE unsigned atomicInc(unsigned *address);
 DEVICE unsigned atomicInc(unsigned *address, unsigned max);
 DEVICE int atomicInc(int *address);
 
-DEVICE int atomicMax(int *address, int val);
-DEVICE unsigned atomicMax(unsigned *address, unsigned val);
-DEVICE unsigned long long atomicMax(unsigned long long *address,
-                                    unsigned long long val);
-
-DEVICE int atomicExch(int *address, int val);
-DEVICE unsigned atomicExch(unsigned *address, unsigned val);
-DEVICE unsigned long long atomicExch(unsigned long long *address,
-                                     unsigned long long val);
-
-DEVICE unsigned atomicCAS(unsigned *address, unsigned compare, unsigned val);
-DEVICE int atomicCAS(int *address, int compare, int val);
-DEVICE unsigned long long atomicCAS(unsigned long long *address,
-                                    unsigned long long compare,
-                                    unsigned long long val);
+namespace {
+
+template <typename T> DEVICE T atomicAdd(T *address, T val) {
+  return __atomic_fetch_add(address, val, __ATOMIC_SEQ_CST);
+}
+
+template <typename T> DEVICE T atomicMax(T *address, T val) {
+  return __atomic_fetch_max(address, val, __ATOMIC_SEQ_CST);
+}
+
+template <typename T> DEVICE T atomicExch(T *address, T val) {
+  T r;
+  __atomic_exchange(address, &val, &r, __ATOMIC_SEQ_CST);
+  return r;
+}
+
+template <typename T> DEVICE T atomicCAS(T *address, T compare, T val) {
+  (void)__atomic_compare_exchange(address, &compare, &val, false,
+                                  __ATOMIC_SEQ_CST, __ATOMIC_RELAXED);
+  return compare;
+}
 
+} // namespace
 #endif