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

[Xiuli PAN via cfe-commits]

pxli168 added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:593
@@ -592,2 +592,3 @@
+def err_no_declarators : Error<"declaration does not declare anything">;
 def ext_typedef_without_a_name : ExtWarn<"typedef requires a name">,
   InGroup<MissingDeclarations>;
----------------
Anastasia wrote:
> I just can't understand the intention here. Could you give any code example or reference to spec?
I will try,

================
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">;
----------------
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.

================
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">;
----------------
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.

================
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)
----------------
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.

================
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())
----------------
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.

================
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()))
----------------
Anastasia wrote:
> Could you remove this comment?
OK. but the "|" may seems strange. I just want to explain it.

================
Comment at: lib/Sema/SemaExpr.cpp:7550
@@ -7527,3 +7549,3 @@
                                   LHSType, RHSVecType->getElementType(),
-                                  RHSType))
+                                  RHSType, Loc))
       return RHSType;
----------------
Anastasia wrote:
> I am not clear about the purpose of this change.
That seems some dead experiment. ):

================
Comment at: lib/Sema/SemaExpr.cpp:10115
@@ +10114,3 @@
+    // Block.
+    if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+        Result->isBlockPointerType()) {
----------------
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.

================
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
----------------
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.

================
Comment at: lib/Sema/SemaInit.cpp:6155
@@ +6154,3 @@
+                       TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+    if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+        Args.size() > 0) {
----------------
Anastasia wrote:
> Should you check if it's a program scope variable? Or alternatively we could disallow non-pointer variables with global AS, which should have already been done already.
It seems so. I will have a try.

================
Comment at: test/CodeGenOpenCL/opencl_types.cl:39
@@ -38,3 @@
-
-void __attribute__((overloadable)) bad1(image1d_t *b, image2d_t *c, image2d_t *d) {}
-// CHECK-LABEL: @{{_Z4bad1P11ocl_image1dP11ocl_image2dS2_|"\\01\?bad1@@\$\$J0YAXPE?APAUocl_image1d@@PE?APAUocl_image2d@@1 at Z"}}
----------------
Anastasia wrote:
> I see. Would it reduce test coverage? Was it potentially testing the mangling of image types?
> 
> I think it's best to change the code instead of completely removing it. May be just remove * so it will be parsed. We seem not to have any test for mangling the images.
> 
Thank you advice, if may test for the mangling.
But the name bad make me the think it is a useless test so I just remove it.

I will change them into no-pointer type.

================
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}}
+}
----------------
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.

================
Comment at: test/SemaOpenCL/invalid-decl.cl:6
@@ +5,2 @@
+  myfun(); // expected-error {{implicit declaration of function 'myfun' is invalid in OpenCL}}
+}
----------------
Anastasia wrote:
> Yes, I think having it in parser is wrong. Not sure why it's there. You can move it if you wish. Otherwise, I will do afterwards.
Keep them be together, and easy to move to right place then.


http://reviews.llvm.org/D16047