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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
