[PATCH] D47618: __c11_atomic_load's _Atomic can be const

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: yaxunl.
jfb added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:3361
+    if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
         AtomTy.getAddressSpace() == LangAS::opencl_constant) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
----------------
rsmith wrote:
> 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.)
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?


================
Comment at: lib/Sema/SemaChecking.cpp:3368-3374
   } else if (Form != Load && Form != LoadCopy) {
     if (ValType.isConstQualified()) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_pointer)
         << Ptr->getType() << Ptr->getSourceRange();
       return ExprError();
     }
   }
----------------
rsmith wrote:
> 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?
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.


Repository:
  rC Clang

https://reviews.llvm.org/D47618