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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to