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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 16 inline comments as done.
yaxunl 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")
+
----------------
Anastasia wrote:
> What about min/max? I believe they will need to have the scope too. 
They are not 2.0 atomic builtin functions. They can be implemented as library functions through 2.0 atomic builtin functions.


================
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">;
----------------
Anastasia wrote:
> Btw, is this disallowed by the spec? Can't find anything relevant.
Just temporarily not supported by Clang. Will add support later.


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


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


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


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


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


================
Comment at: lib/Sema/SemaChecking.cpp:3146
+            Op == AtomicExpr::AO__opencl_atomic_load)
+                ? 0
+                : 1);
----------------
Anastasia wrote:
> formatting seems odd.
this is done by clang-format


================
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
----------------
Anastasia wrote:
> GEN4 -> SPIR
will change


================
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
+
----------------
Anastasia wrote:
> GEN0 -> AMDGPU
Actually this triple is armv5e. This test requires a target not supporting atomic instructions. Will change GEN0 -> ARM


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


================
Comment at: test/CodeGenOpenCL/atomic-ops.cl:7
+
+#ifndef ALREADY_INCLUDED
+#define ALREADY_INCLUDED
----------------
Anastasia wrote:
> why do we need this?
This is to test the builtin works in pch. When generating pch, ALREADY_INCLUDED is undefined, therefore pch will include all functions. When including pch, since ALREADY_INCLUDED is defined through pch, the cl file is empty and function definitions from pch is used for codegen.


================
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);
+}
----------------
Anastasia wrote:
> I think we could use different scope types all over...
will add them.


================
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
----------------
Anastasia wrote:
> do we have an addrspacecast before?
No,


================
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)}}
----------------
Anastasia wrote:
> could we have the full error for consistency?
will do


================
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)}}
+
----------------
Anastasia wrote:
> could we also test with constant AS? Also I would generally improve testing wrt address spaces...
will add tests for different addr spaces.


https://reviews.llvm.org/D28691