__c11_atomic_load's _Atomic can be const

[jfb]

Summary

C++11 onwards specs the non-member functions atomic_load and atomic_load_explicit as taking the atomic<T> by const (potentially volatile) pointer. C11, in its infinite wisdom, decided to drop the const, and C17 will fix this with DR459 (the current draft forgot to fix B.16, but that’s not the normative part).

clang’s lib/Headers/stdatomic.h implements these as #define to the __c11_* equivalent, which are builtins with custom typecheck. Fix the typecheck.

D47613 takes care of the libc++ side.

Discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058129.html

rdar://problem/27426936

[jfb]

Note that I don't touch the OpenCL __constant stuff, because the separate address space means this can be actually read-only, which means you can't cmpxchg for very wide reads.

[rsmith]

We need to figure out what should happen in the OpenCL case, but the rest seems fine.

lib/Sema/SemaChecking.cpp

3360 ↗

(On Diff #149397)

The LoadCopy check is redundant; only the GNU __atomic_load builtin has the LoadCopy form. But see below, we can avoid this duplicated condition with some refactoring.

3361 ↗

(On Diff #149397)

We also need to figure out what to do about this -- should an atomic load from a constant address space be valid? (It seems a little pointless to use an *atomic* load here, but not obviously wrong.)

3368–3374 ↗

(On Diff #149397)

It would be a little nicer to change this else if to a plain if and conditionalize the diagnostic instead.

Can you track down whoever added the address space check to the C11 atomic path and ask them if they really meant for it to not apply to the GNU atomic builtins?

[jfb]

lib/Sema/SemaChecking.cpp

3361 ↗

(On Diff #149397)

I think it's mostly harmless except when the address space is actually constant (not C++ constant) because in these cases a wide atomic might need cmpxchg to perform a read, and that will fail writing. Maybe @Anastasia can chime in for OpenCL?

3368–3374 ↗

(On Diff #149397)

It was @yaxunl in https://reviews.llvm.org/D28691
Nobody asked about GNU atomic builtins with OpenCL in that review. I'll let @yaxunl chime in.

[yaxunl]

lib/Sema/SemaChecking.cpp

3368–3374 ↗

(On Diff #149397)

Your change LGTM. Originally the check for constant addr space was introduced to match the check of C11 for const. OpenCL itself does not have this restriction.

[Anastasia]

test/SemaOpenCL/atomic-ops.cl

61 ↗

(On Diff #149397)

Could we add a line with constant AS pointer:

__opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group);

Just to make sure we still give an error for this case.

[jfb]

• Add constant AS pointer test.

[jfb]

Give the comments, I think this is ready to commit.

[Anastasia]

LGTM from OpenCL side! Thanks!

cfe/trunk/lib/Sema/SemaChecking.cpp

ExprResult Sema::SemaAtomicOpsOverloaded(ExprResult TheCallResult,
   QualType AtomTy = pointerType->getPointeeType(); // 'A'
   QualType ValType = AtomTy; // 'C'
   if (IsC11) {
     if (!AtomTy->isAtomicType()) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_atomic)
         << Ptr->getType() << Ptr->getSourceRange();
       return ExprError();
     }
-    if (AtomTy.isConstQualified() ||
+    if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
         AtomTy.getAddressSpace() == LangAS::opencl_constant) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
           << (AtomTy.isConstQualified() ? 0 : 1) << Ptr->getType()
           << Ptr->getSourceRange();
       return ExprError();
     }
     ValType = AtomTy->getAs<AtomicType>()->getValueType();
   } else if (Form != Load && Form != LoadCopy) {

cfe/trunk/test/Sema/atomic-ops.c

void f(_Atomic(int) *i, const _Atomic(int) *ci,
   __c11_atomic_store(0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
   __c11_atomic_store((int*)0,0,0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
   __c11_atomic_store(i, 0, memory_order_relaxed);
   __c11_atomic_store(ci, 0, memory_order_relaxed); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
 
   __c11_atomic_load(i, memory_order_seq_cst);
   __c11_atomic_load(p, memory_order_seq_cst);
   __c11_atomic_load(d, memory_order_seq_cst);
-  __c11_atomic_load(ci, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_seq_cst);
 
   int load_n_1 = __atomic_load_n(I, memory_order_relaxed);
   int *load_n_2 = __atomic_load_n(P, memory_order_relaxed);
   float load_n_3 = __atomic_load_n(D, memory_order_relaxed); // expected-error {{must be a pointer to integer or pointer}}
   __atomic_load_n(s1, memory_order_relaxed); // expected-error {{must be a pointer to integer or pointer}}
   load_n_1 = __atomic_load_n(CI, memory_order_relaxed);
 
   __atomic_load(i, I, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
void f(_Atomic(int) *i, const _Atomic(int) *ci,
   (void)(int)__atomic_test_and_set(&flag_k, memory_order_seq_cst); // expected-warning {{passing 'const volatile int *' to parameter of type 'volatile void *'}}
   (void)(int)__atomic_test_and_set(&flag, memory_order_seq_cst);
   __atomic_clear(&flag_k, memory_order_seq_cst); // expected-warning {{passing 'const volatile int *' to parameter of type 'volatile void *'}}
   __atomic_clear(&flag, memory_order_seq_cst);
   (int)__atomic_clear(&flag, memory_order_seq_cst); // expected-error {{operand of type 'void'}}
 
   __c11_atomic_init(ci, 0); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
   __c11_atomic_store(ci, 0, memory_order_release); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
-  __c11_atomic_load(ci, memory_order_acquire); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(ci, memory_order_acquire);
 
   // Ensure the <stdatomic.h> macros behave appropriately.
   atomic_int n = ATOMIC_VAR_INIT(123);
   atomic_init(&n, 456);
   atomic_init(&n, (void*)0); // expected-warning {{passing 'void *' to parameter of type 'int'}}
 
   const atomic_wchar_t cawt;
   atomic_init(&cawt, L'x'); // expected-error {{non-const}}

cfe/trunk/test/SemaOpenCL/atomic-ops.cl

void f(atomic_int *i, const atomic_int *ci,
   __opencl_atomic_store(i_g, 0, memory_order_relaxed, memory_scope_work_group);
   __opencl_atomic_store(i_l, 0, memory_order_relaxed, memory_scope_work_group);
   __opencl_atomic_store(i_p, 0, memory_order_relaxed, memory_scope_work_group);
   __opencl_atomic_store(i_c, 0, memory_order_relaxed, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); // 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)}}
+  __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
   (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
void f(atomic_int *i, const atomic_int *ci,
   bool cmpexchw_3 = __opencl_atomic_compare_exchange_weak(d, I, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_group); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__generic float *'}}
   (void)__opencl_atomic_compare_exchange_weak(i, CI, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_group); // expected-warning {{passing 'const __generic int *' to parameter of type '__generic int *' discards qualifiers}}
 
   // Pointers to different address spaces are allowed.
   bool cmpexch_10 = __opencl_atomic_compare_exchange_strong((global atomic_int *)0x308, (constant int *)0x309, 1, memory_order_seq_cst, memory_order_seq_cst, memory_scope_work_group);
 
   __opencl_atomic_init(ci, 0); // 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)}}
   __opencl_atomic_store(ci, 0, memory_order_release, memory_scope_work_group); // 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)}}
-  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group); // 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)}}
+  __opencl_atomic_load(ci, memory_order_acquire, memory_scope_work_group);
 
   __opencl_atomic_init(&gn, 456);
   __opencl_atomic_init(&gn, (void*)0); // expected-warning{{incompatible pointer to integer conversion passing '__generic void *' to parameter of type 'int'}}
 }
 
 void memory_checks(atomic_int *Ap, int *p, int val) {
   // non-integer memory order argument is casted to integer type.
   (void)__opencl_atomic_load(Ap, 1.0f, memory_scope_work_group);