[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")
----------------
Should HIP also consider adding a __hip_atomic_load and __hip_atomic_store?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9161-9179
+  case SyncScope::HIPSingleThread:
+    Name = "singlethread";
+    break;
+  case SyncScope::HIPWavefront:
+  case SyncScope::OpenCLSubGroup:
+    Name = "wavefront";
+    break;
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:4877
   bool IsPassedByAddress = false;
-  if (!IsC11 && !IsN) {
+  if (!IsC11 && !IsHIP && !IsN) {
     ByValType = Ptr->getType();
----------------
Does HIP have atomic types for this to be meaningful? I would expect that HIP should be treated the same as OpenCL which does not appear to be here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87858