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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Looks like you haven't introduced proper constants in the header for scopes.



================
Comment at: docs/LanguageExtensions.rst:1855
+atomic builtins are in ``explicit`` form of the corresponding OpenCL 2.0
+builtin function, and are named with a ``__opencl__`` prefix.)
 
----------------
1. "an", not "in".
2. There's no need to put "explicit" in `` quotes.
3. The prefix you're using is "__opencl_", with only one trailing underscore.


================
Comment at: lib/CodeGen/CGAtomic.cpp:736
     OrderFail = EmitScalarExpr(E->getOrderFail());
-    if (E->getNumSubExprs() == 6)
+    if (E->getNumSubExprs() == 7)
       IsWeak = EmitScalarExpr(E->getWeak());
----------------
This is equivalent to checking for the two AO kinds, right?  Probably better to just do that.


================
Comment at: lib/CodeGen/CGExpr.cpp:50
+llvm::Value *CodeGenFunction::EmitCastToVoidPtr(llvm::Value *value,
+                                                bool CastToGenericAddrSpace) {
+  unsigned addressSpace;
----------------
I think it would be better to keep this function simple and just add the cast outside in the two places you need it.

Also, please remember to use the target hook instead of using an addrspacecast directly.


================
Comment at: lib/Sema/SemaChecking.cpp:3046
       // The order(s) are always converted to int.
       Ty = Context.IntTy;
     }
----------------
This clause now covers the scope argument as well.


================
Comment at: lib/Sema/SemaChecking.cpp:3060
+  if (IsOpenCL) {
+    Scope = TheCall->getArg(TheCall->getNumArgs() - 1);
+  } else {
----------------
You need to check that is a constant expression in the correct range, and you should add a test for that.


================
Comment at: lib/Sema/SemaChecking.cpp:3062
+  } else {
+    Scope = IntegerLiteral::Create(
+        Context, llvm::APInt(Context.getTypeSize(Context.IntTy), (uint64_t)1),
----------------
Please document the meaning of 1.


https://reviews.llvm.org/D28691