[PATCH] D87858: [hip] Add HIP scope atomic ops.

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.def:792-799
+ATOMIC_BUILTIN(__hip_atomic_compare_exchange_strong, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_exchange, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_fetch_add, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_fetch_and, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_fetch_or, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_fetch_xor, "v.", "t")
+ATOMIC_BUILTIN(__hip_atomic_fetch_min, "v.", "t")
----------------
t-tye wrote:
> Should HIP also consider adding a __hip_atomic_load and __hip_atomic_store?
What is being done for the memory fences?

Does HIP have any interest in adding a memory order?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9161-9179
+  case SyncScope::HIPSingleThread:
+    Name = "singlethread";
+    break;
+  case SyncScope::HIPWavefront:
+  case SyncScope::OpenCLSubGroup:
+    Name = "wavefront";
+    break;
----------------
t-tye wrote:
> The HIP memory model uses a single happens-before relation for all address spaces which is different to OpenCL. So these need to use the "*-one-as" names.
> 
> Before committing this we should decide if we want to implement the OpenCL address space fence support using the syncScope, in which case we may want to adjust this name before we start using it.
Actually the other way round, OpenCL should be using the  "*-one-as" names except for the seq_cst memory ordering which is all address spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87858