[PATCH] D16047: [OpenCL] Add Sema checks for OpenCL 2.0

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7670
@@ +7669,3 @@
+  "%0 can only be used as a function parameter">;
+def err_opencl_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global address space in opencl">;
----------------
pxli168 wrote:
> Anastasia wrote:
> > Could you do something like:
> > 
> >     def err_atomic_init_constant : Error<
> >       "atomic variable can only be %select{assigned|initialised}0 to a compile time constant"
> >       " in the declaration statement in the program scope">;
> > 
> Seems good. But that error message was from spec, this will change it.
You can pick the wording you like. I personally find this one better!

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7674
@@ +7673,3 @@
+  "invalid block prototype, variadic arguments are not allowed in opencl">;
+def err_opencl_invalid_block_array : Error<
+  "array of block is invalid in OpenCL">;
----------------
pxli168 wrote:
> Anastasia wrote:
> > Could we combine err_opencl_invalid_block_array and err_opencl_pointer_to_image saying something like:
> > 
> > "Declaring a %select{pointer|array}0 of type %1 is not allowed in OpenCL"
> OK, it will save few lines.
And more importantly won't populate diagnostics.

================
Comment at: lib/Sema/SemaDecl.cpp:5724
@@ +5723,3 @@
+      R->isPipeType()) {
+    Diag(D.getIdentifierLoc(),
+         diag::err_opencl_type_can_only_be_used_as_function_parameter)
----------------
pxli168 wrote:
> Anastasia wrote:
> > Some of them have to be. For example for C types that we use differently in CL. But for CL only types do we need to check additionally that it's CL? Do we not know it already?
> Yes, but it seems all old code write in that way. I just follow the style.
Ok, I think an improvement to the old code is always welcome!

================
Comment at: lib/Sema/SemaExpr.cpp:6299
@@ -6286,3 +6298,3 @@
   // Now check the two expressions.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType())
----------------
pxli168 wrote:
> Anastasia wrote:
> > I am not sure what the question is?
> > 
> > I think using block in a condition should be disallowed. Could you add this to testing as well? 
> I don't see any thing talk about the block and condition. Spec only tells about block and selection.
Ok, in that case there is nothing left to do here I guess.

================
Comment at: lib/Sema/SemaExpr.cpp:6316
@@ +6315,3 @@
+  if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200) {
+    // should output error for both LHS and RHS, use | instead ||
+    if (checkBlockType(*this, LHS.get()) | checkBlockType(*this, RHS.get()))
----------------
pxli168 wrote:
> Anastasia wrote:
> > Could you remove this comment?
> OK. but the "|" may seems strange. I just want to explain it.
Doesn't seem strange to me.

================
Comment at: lib/Sema/SemaExpr.cpp:10115
@@ +10114,3 @@
+    // Block.
+    if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+        Result->isBlockPointerType()) {
----------------
pxli168 wrote:
> Anastasia wrote:
> > The code above seems to do similar. Could we combine into one function/diagnostic?
> Yes, they are. But they live in different function test for operand "&" and "*" and maybe it is hard to merge them together. I have uploaded the full diff as you asked and uou can expand the code to see they are in two function check for the two unary operators.
Yes, it's not a problem that they are in different functions. Could this code be encapsulated in a function, let's say
    checkBlockType(...)

 and be called from multiple places?

================
Comment at: lib/Sema/SemaInit.cpp:6139
@@ +6138,3 @@
+  // OpenCL v2.0 s6.13.11.1 - The ATOMIC_VAR_INIT macro expands to a token
+  // sequence suitable for initializing an atomic object of a type that is
+  // initialization-compatible with value. An atomic object with automatic
----------------
pxli168 wrote:
> Anastasia wrote:
> > I don't think we need to copy the spec text here. I would like to see some explanation of the code actually.
> > 
> > Could you write something like: "Non-program scope atomic variables can't be initialized."
> But the example said there can be program scope atomic that only in global address space.
> 
> > This macro can only be used to initialize atomic objects that are declared in program scope in the global address space
> 
> The quote from spec said so.
Yes, so copying spec into code is not the goal. Let's try to make it shorter and still have the same meaning!

================
Comment at: test/Parser/opencl-atomics-cl20.cl:71
@@ +70,3 @@
+void atomic_init_test() {
+    atomic_int guide = ATOMIC_VAR_INIT(42); // expected-error {{initialization of atomic variables is restricted to variables in global address space in opencl}}
+}
----------------
pxli168 wrote:
> Anastasia wrote:
> > Why using a macro here? It seems to complicate the test without adding any functionality.
> Because the the init type maybe different and then have another error.
> 
> I will have a test for this semacheck as I said above.
The macro is removed by the preprocessor and it's empty here anyways.

You are not adding a builtin macro ATOMIC_VAR_INIT to Clang.

This test can look a lot cleaner (without doing any macro expansion):
atomic_int guide = 42;


http://reviews.llvm.org/D16047