[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

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

28

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

21

There must be an existing builtin for this?

[JonChesterfield]

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

796

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

21

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/CMakeLists.txt

set(cuda_sources
   ${devicertl_base_directory}/common/src/omp_data.cu
   ${devicertl_base_directory}/common/src/omptarget.cu
   ${devicertl_base_directory}/common/src/parallel.cu
   ${devicertl_base_directory}/common/src/reduction.cu
   ${devicertl_base_directory}/common/src/support.cu
   ${devicertl_base_directory}/common/src/sync.cu
   ${devicertl_base_directory}/common/src/task.cu)
 
+# Functions implemented in IR are used where there is not yet a corresponding
+# intrinsic available in clang. The intent is for these functions to be removed
+# as clang is extended.
+set(llvm_sources
+  ${CMAKE_CURRENT_SOURCE_DIR}/src/amdgcn_atomic.ll)
+
 set(h_files
   ${CMAKE_CURRENT_SOURCE_DIR}/src/amdgcn_interface.h
   ${CMAKE_CURRENT_SOURCE_DIR}/src/hip_atomics.h
   ${CMAKE_CURRENT_SOURCE_DIR}/src/target_impl.h
   ${devicertl_base_directory}/common/debug.h
   ${devicertl_base_directory}/common/device_environment.h
   ${devicertl_base_directory}/common/omptarget.h
   ${devicertl_base_directory}/common/omptargeti.h
macro(add_cuda_bc_library)
   list(APPEND bc_files linkout.cuda.${mcpu}.bc)
 endmacro()
 
 set(libname "omptarget-amdgcn")
 
 foreach(mcpu ${mcpus})
   set(bc_files)
   add_cuda_bc_library(${cuda_sources})
+  list(APPEND bc_files ${llvm_sources})
 
   set(bc_libname lib${libname}-${mcpu}.bc)
   add_custom_command(
     OUTPUT ${bc_libname}
     COMMAND ${AOMP_BINDIR}/llvm-link ${bc_files} | ${AOMP_BINDIR}/opt --always-inline -o ${OUTPUTDIR}/${bc_libname}
     DEPENDS ${bc_files})
 
   add_custom_target(lib${libname}-${mcpu} ALL DEPENDS ${bc_libname})
 
   install(FILES ${OUTPUTDIR}/${bc_libname}
      DESTINATION "${OPENMP_INSTALL_LIBDIR}/libdevice"
   )
 endforeach()

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

+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+target triple = "amdgcn-amd-amdhsa"
+
+; These functions are implemented in IR as there is not yet a corresponding intrinsic
+; available in clang. The intent is to remove it once said intrinsic is implemented.
+
+declare i32 @llvm.amdgcn.atomic.inc.i32.p0i32(i32* nocapture, i32, i32, i32, i1) #1
+
+; Function Attrs: alwaysinline nounwind
+define i32 @__amdgcn_atomic_inc_u32(i32* %x, i32 %v) #0 {
+entry:
+  %ret = call i32 @llvm.amdgcn.atomic.inc.i32.p0i32(i32* %x, i32 %v,
+  i32 5, ; Ordering. AtomicOrdering.h: sequentially consistent
+  i32 2, ; Scope. SyncScope.h:  OpenCLAllSVMDevices is 2
+  i1 0 ; Volatile. False for consistency with other atomic operations
+  )
+  ret i32 %ret
+}
+
+attributes #0 = { alwaysinline nounwind }
+attributes #1 = { nounwind argmemonly }

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);
-
-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);
+}
+
+// This functions (implemented in amdgcn_atomic.ll) is intended to
+// be replaced with clang intrinsics once said intrinsics are implemented
+
+EXTERN uint32_t __amdgcn_atomic_inc_u32(uint32_t *, uint32_t);
+INLINE uint32_t atomicInc(uint32_t *address, uint32_t val) {
+  return __amdgcn_atomic_inc_u32(address, val);
+}
+
+template <typename T> DEVICE T atomicMax(T *address, T val) {
+  return __atomic_max_fetch(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

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

if (gtid == 0)
       *Buffer = 0; // Reset to minimum loop iteration value.
 
     // Barrier.
     syncWorkersInGenericMode(NumThreads);
 
     // Atomic max of iterations.
     uint64_t *varArray = (uint64_t *)array;
     uint64_t elem = varArray[i];
-    (void)__kmpc_atomic_max((unsigned long long int *)Buffer,
-                            (unsigned long long int)elem);
+    (void)__kmpc_atomic_max(Buffer, elem);
 
     // Barrier.
     syncWorkersInGenericMode(NumThreads);
 
     // Read max value and update thread private array.
     varArray[i] = *Buffer;
 
     // Barrier.
     syncWorkersInGenericMode(NumThreads);
   }
 }