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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/Basic/Builtins.def:713
+ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t")
+ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t")
+
----------------
What about min/max? I believe they will need to have the scope too. 


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6956
+  "synchronization scope argument to atomic operation is invalid">;
+def err_atomic_op_has_non_constant_synch_scope : Error<
+  "non-constant synchronization scope argument to atomic operation is not supported">;
----------------
Btw, is this disallowed by the spec? Can't find anything relevant.


================
Comment at: lib/CodeGen/CGAtomic.cpp:678
 RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
+  bool IsOpenCL = E->isOpenCL();
   QualType AtomicTy = E->getPtr()->getType()->getPointeeType();
----------------
Seems short enough to introduce an extra variable here. :)


================
Comment at: lib/CodeGen/CGAtomic.cpp:707
 
-  switch (E->getOp()) {
+  auto Op = E->getOp();
+  switch (Op) {
----------------
The same here... not sure adding an extra variable is helping here. :)


================
Comment at: lib/CodeGen/CGAtomic.cpp:889
+        return V;
+      auto OrigLangAS = E->getType()
+                            .getTypePtr()
----------------
Formatting seems to be a bit odd here...


================
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+      "Non-constant synchronization scope not supported");
+  auto sco = (llvm::SynchronizationScope)(
+      cast<llvm::ConstantInt>(Scope)->getZExtValue());
----------------
Variable name doesn't follow the style.


================
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+      "Non-constant synchronization scope not supported");
+  auto sco = (llvm::SynchronizationScope)(
+      cast<llvm::ConstantInt>(Scope)->getZExtValue());
----------------
Anastasia wrote:
> Variable name doesn't follow the style.
could we avoid using C style cast?


================
Comment at: lib/Sema/SemaChecking.cpp:3146
+            Op == AtomicExpr::AO__opencl_atomic_load)
+                ? 0
+                : 1);
----------------
formatting seems odd.


================
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:1
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64 -emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
----------------
GEN4 -> SPIR


================
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:2
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64 -emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
+
----------------
GEN0 -> AMDGPU


================
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:4
+
+void f(atomic_int *i, int cmp) {
+  int x;
----------------
Could we use different scopes?


================
Comment at: test/CodeGenOpenCL/atomic-ops.cl:7
+
+#ifndef ALREADY_INCLUDED
+#define ALREADY_INCLUDED
----------------
why do we need this?


================
Comment at: test/CodeGenOpenCL/atomic-ops.cl:15
+  // CHECK: load atomic i32, i32 addrspace(4)* %{{[.0-9A-Z_a-z]+}} singlethread seq_cst
+  int x = __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_item);
+}
----------------
I think we could use different scope types all over...


================
Comment at: test/CodeGenOpenCL/atomic-ops.cl:32
+  // CHECK-LABEL: @fi4(
+  // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32 addrspace(4)* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]] singlethread acquire acquire
+  // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = extractvalue { i32, i1 } [[PAIR]], 0
----------------
do we have an addrspacecast before?


================
Comment at: test/SemaOpenCL/atomic-ops.cl:22
+       intptr_t *P, float *D, struct S *s1, struct S *s2) {
+  __opencl_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+  __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
----------------
could we have the full error for consistency?


================
Comment at: test/SemaOpenCL/atomic-ops.cl:30
+  __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+  __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
+
----------------
could we also test with constant AS? Also I would generally improve testing wrt address spaces...


https://reviews.llvm.org/D28691