[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.


================
Comment at: include/clang/Basic/SyncScope.h:23
+enum class SyncScope {
+  OpenCLWorkItem = 0,
+  OpenCLWorkGroup = 1,
----------------
The OpenCL workitem scope is only used for image fences and does not apply to atomic operations so not sure that it should be in this enumeration which is used only for memory atomics.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7554-7555
+  switch (S) {
+  case SyncScope::OpenCLWorkItem:
+    Name = "singlethread";
+    break;
----------------
OpenCL only uses workitem for image fences which are not the same as atomic memory fences.

For image fences I don't think it would map to singlethread which is intended for signal handler support to ensure a signal handler has visibility of the updates done by a thread which is more of an optimization barrier. In contrast an image fence may need to flush caches to make the image and vector access paths coherent in a single thread.

Since this patch does not support fences probably want to leave workitem scope out. Current AMDGPU targets do not need to do anything for an OpenCL image fence, but other targets may need to generate an intrinsic since there is no LLVMIR instruction for this.


================
Comment at: lib/Headers/opencl-c.h:13145-13150
   memory_scope_work_item,
   memory_scope_work_group,
   memory_scope_device,
   memory_scope_all_svm_devices,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
   memory_scope_sub_group
----------------
Do these have to have the same values as the SycnScope enumeration? Should that be ensured in a similar way to the memory_order enumeration?


================
Comment at: lib/Sema/SemaChecking.cpp:3103
+        Diag(Scope->getLocStart(),
+             diag::err_atomic_op_has_non_constant_synch_scope)
+            << Scope->getSourceRange();
----------------
IIRC OpenCL allows the scope to be a runtime value. So will doing this will likely cause failures in conformance?


https://reviews.llvm.org/D28691