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