[OpenCL] Apply missing restrictions for Blocks in OpenCL v2.0

[Anastasia]

Summary

Applying the following restrictions for block variables in OpenCL (v2.0 s6.12.5):

• __block storage class is disallowed
• variadic arguments are disallowed
• every block declaration must be const qualified and initialized
• a block can't be used as a return type of a function
• extern speficier is disallowed

This adds diagnostics missing from http://reviews.llvm.org/D16047

[Leporacanthicus]

include/clang/Basic/DiagnosticSemaKinds.td

7684

Is this blank line there for a reason, as there seem to be none above...

7704

US spelling is initialized - which I believe is standard for Clang?

test/SemaOpenCL/invalid-blocks-cl20.cl

6

US spelling will need to be fixed here too, if fixed above.

[pxli168]

lib/Sema/SemaDecl.cpp

13011

can -> cannot
I just find I could not understand this.

lib/Sema/SemaType.cpp

3827

OpenCL v1.2 s6.9 doesn't support variadic functions, except for ->OpenCL v1.2 s6.9 - Variadic functions are not supported, ....

[Anastasia]

Update according to review comments!

[pekka.jaaskelainen]

test/SemaOpenCL/invalid-func.cl

6 ↗

(On Diff #47823)

I wonder should we check for the fingerprint too when letting printf() pass?
This is not the correct OpenCL printf() as the first argument should be in constant address space.

[MatsPetersson]

include/clang/Basic/DiagnosticSemaKinds.td

7700

Looks good

7717

Looks good now (from my official account this time! :) )

[MatsPetersson]

test/SemaOpenCL/invalid-func.cl

6 ↗

(On Diff #47823)

Good spot, this should be fixed.

[Anastasia]

Drafted code for printf handling.

Made me think about:

1. How much signature check should we do i.e. should we check the pointer AS itself (generic for CL2.0, any other otherwise) or qualifiers being used for the first parameter (const, volatile ...)?
2. Would it be nicer to just add printf as a Clang OpenCL builitin to prevent it being declared in various ways and avoid writing custom signature check as in this change?

[MatsPetersson]

I think adding printf as a builtin function is a good idea.

If not, I feel that the error message should distinguish between (for example):

int printf(global char* fmt, ...);

and

void myfunc(int n, ...);

since the former is "incorrect prototype of printf" and the latter is "not a valid opencl function".

[Anastasia]

@Mats, I think I would prefer not to add even more special handling (i.e. error in this case) for printf.

I will look into adding it as a Builtin instead. However, I would prefer to remove it from this change and submit a separate patch as it doesn't belong to Clang blocks anyways.

Thanks!

[Anastasia]

@Mats, I think I would prefer not to add even more special handling (i.e. error in this case) for printf.

I will look into adding it as a Builtin instead. However, I would prefer to remove it from this change and submit a separate patch as it doesn't belong to Clang blocks anyways.

Thanks!

[MatsPetersson]

@Anastasia: Yes, I agree, printf should be done as a separate patch.

[Anastasia]

Removed varargs due to printf special handling. To be added as a separate review!

[pxli168]

This comment was removed by pxli168.

[pxli168]

LGTM!
Thanks

[pxli168]

It seems this patch will check block for CL1.2 or eailer? But the spec reference is for OpenCL v2.0.

[Anastasia]

Yes, that's right. In this commit I assume parser has already accepted the blocks.

We currently accept them if -fblocks is passed. I think we should also accept blocks by default with CL2.0. But I am thinking not to restrict passing -fblocks with earlier CL version. This way it's possible to enable them with earlier standards if whoever does it knows what it's needed for. We could give a warning in this case. What do you think?

However, I wouldn't block this patch on it, as I wouldn't commit them together anyways. But I am happy to prepare a separate patch.

[pxli168]

Then there will be no problem.
I will change my patches as well.
LGTM!
Thanks.

[Anastasia]

r262616

include/clang/Basic/DiagnosticSemaKinds.td

   "declaring variable of type %0 is not allowed">;
 def err_opencl_half_param : Error<
   "declaring function parameter of type %0 is not allowed; did you forget * ?">;
-def err_opencl_half_return : Error<
-  "declaring function return value of type %0 is not allowed; did you forget * ?">;
+def err_opencl_invalid_return : Error<
+  "declaring function return value of type %0 is not allowed %select{; did you forget * ?|}1">;
 def warn_enum_value_overflow : Warning<"overflow in enumeration value">;
 def warn_pragma_options_align_reset_failed : Warning<
   "#pragma options align=reset failed: %0">,

   "field of illegal %select{type|pointer type}0 %1 declared here">;
 def err_event_t_global_var : Error<
   "the event_t type cannot be used to declare a program scope variable">;
-def err_event_t_struct_field : Error<
-  "the event_t type cannot be used to declare a structure or union field">;
+def err_opencl_type_struct_or_union_field : Error<
+  "the %0 type cannot be used to declare a structure or union field">;
 def err_event_t_addr_space_qual : Error<
   "the event_t type can only be used with __private address space qualifier">;
 def err_expected_kernel_void_return_type : Error<

 def err_opencl_unknown_type_specifier : Error<
   "OpenCL does not support the '%0' %select{type qualifier|storage class specifier}1">;
 
+// OpenCL v2.0 s6.12.5 Blocks restrictions
+def err_opencl_block_storage_type : Error<
+  "the __block storage type is not permitted">;
+def err_opencl_invalid_block_declaration : Error<
+  "invalid block variable declaration - must be %select{const qualified|initialized}0">;
+def err_opencl_extern_block_declaration : Error<
+  "invalid block variable declaration - using 'extern' storage class is disallowed">;
+
 } // end of sema category
 
 let CategoryName = "OpenMP Issue" in {

lib/Sema/SemaDecl.cpp

     return;
   }
 
-  // OpenCL v1.2 s6.8 -- The static qualifier is valid only in program
+  // OpenCL v1.2 s6.8 - The static qualifier is valid only in program
   // scope.
   if (getLangOpts().OpenCLVersion == 120 &&
       !getOpenCLOptions().cl_clang_storage_class_specifiers &&

     return;
   }
 
-  // OpenCL v1.2 s6.5 - All program scope variables must be declared in the
-  // __constant address space.
-  // OpenCL v2.0 s6.5.1 - Variables defined at program scope and static
-  // variables inside a function can also be declared in the global
-  // address space.
   if (getLangOpts().OpenCL) {
+    // OpenCL v2.0 s6.12.5 - The __block storage type is not supported.
+    if (NewVD->hasAttr<BlocksAttr>()) {
+      Diag(NewVD->getLocation(), diag::err_opencl_block_storage_type);
+      return;
+    }
+    // OpenCL v2.0 s6.12.5 - Any block declaration must be const qualified and
+    // can't use 'extern' storage class.
+    if (T->isBlockPointerType()) {
+      if (!T.isConstQualified()) {
+        Diag(NewVD->getLocation(), diag::err_opencl_invalid_block_declaration)
+            << 0 /*const*/;
+        NewVD->setInvalidDecl();
+        return;
+      }
+      if (NewVD->hasExternalStorage()) {
+        Diag(NewVD->getLocation(), diag::err_opencl_extern_block_declaration);
+        NewVD->setInvalidDecl();
+        return;
+      }
+    }
+    // OpenCL v1.2 s6.5 - All program scope variables must be declared in the
+    // __constant address space.
+    // OpenCL v2.0 s6.5.1 - Variables defined at program scope and static
+    // variables inside a function can also be declared in the global
+    // address space.
     if (NewVD->isFileVarDecl()) {
       if (!T->isSamplerT() &&
           !(T.getAddressSpace() == LangAS::opencl_constant ||

 void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
   if (var->isInvalidDecl()) return;
 
+  if (getLangOpts().OpenCL) {
+    // OpenCL v2.0 s6.12.5 - Every block variable declaration must have an
+    // initialiser
+    if (var->getTypeSourceInfo()->getType()->isBlockPointerType() &&
+        !var->hasInit()) {
+      Diag(var->getLocation(), diag::err_opencl_invalid_block_declaration)
+          << 1 /*Init*/;
+      var->setInvalidDecl();
+      return;
+    }
+  }
+
   // In Objective-C, don't allow jumps past the implicit initialization of a
   // local retaining variable.
   if (getLangOpts().ObjC1 &&

     D.setInvalidType();
   }
 
-  // OpenCL 1.2 spec, s6.9 r:
-  // The event type cannot be used to declare a structure or union field.
-  if (LangOpts.OpenCL && T->isEventT()) {
-    Diag(Loc, diag::err_event_t_struct_field);
+  // OpenCL v1.2 s6.9b,r & OpenCL v2.0 s6.12.5 - The following types cannot be
+  // used as structure or union field: image, sampler, event or block types.
+  if (LangOpts.OpenCL && (T->isEventT() || T->isImageType() ||
+                          T->isSamplerT() || T->isBlockPointerType())) {
+    Diag(Loc, diag::err_opencl_type_struct_or_union_field) << T;
     D.setInvalidType();
   }
 

lib/Sema/SemaType.cpp

       if (T->isHalfType()) {
         if (S.getLangOpts().OpenCL) {
           if (!S.getOpenCLOptions().cl_khr_fp16) {
-            S.Diag(D.getIdentifierLoc(), diag::err_opencl_half_return) << T;
+            S.Diag(D.getIdentifierLoc(), diag::err_opencl_invalid_return)
+                << T << 0 /*pointer hint*/;
             D.setInvalidType(true);
           } 
         } else if (!S.getLangOpts().HalfArgsAndReturns) {

         }
       }
 
+        // OpenCL v2.0 s6.12.5 - A block cannot be the return value of a
+        // function.
+      if (LangOpts.OpenCL && T->isBlockPointerType()) {
+        S.Diag(D.getIdentifierLoc(), diag::err_opencl_invalid_return)
+            << T << 1 /*hint off*/;
+        D.setInvalidType(true);
+      }
+
       // Methods cannot return interface types. All ObjC objects are
       // passed by reference.
       if (T->isObjCObjectType()) {

test/CodeGen/blocks-opencl.cl

 // This used to crash due to trying to generate a bitcase from a cstring
 // in the constant address space to i8* in AS0.
 
-void dummy(float (^op)(float))
-{
+void dummy(float (^const op)(float)) {
 }
 
 // CHECK: i8 addrspace(3)* getelementptr inbounds ([9 x i8], [9 x i8] addrspace(3)* @.str, i32 0, i32 0)
 
 kernel void test_block()
 {
-  float (^X)(float) = ^(float x) { return x + 42.0f; };
+  float (^const X)(float) = ^(float x) {
+    return x + 42.0f;
+  };
   dummy(X);
 }
 

test/SemaOpenCL/event_t.cl

 event_t glb_evt; // expected-error {{the event_t type cannot be used to declare a program scope variable}}
 
 constant struct evt_s {
-  event_t evt;  // expected-error {{the event_t type cannot be used to declare a structure or union field}}
+  event_t evt; // expected-error {{the 'event_t' type cannot be used to declare a structure or union field}}
 } evt_str = {0};
 
 void foo(event_t evt); // expected-note {{passing argument to parameter 'evt' here}}

test/SemaOpenCL/invalid-blocks-cl20.cl

+// RUN: %clang_cc1 %s -verify -fsyntax-only -cl-std=CL2.0 -fblocks
+
+// OpenCL v2.0 s6.12.5
+
+// All blocks declarations must be const qualified and initialized.
+void f1() {
+  int (^bl1)() = ^() {}; // expected-error{{invalid block variable declaration - must be const qualified}}
+  int (^const bl2)(); // expected-error{{invalid block variable declaration - must be initialized}}
+  int (^const bl3)() = ^const(){
+  };
+}
+
+// A block with extern storage class is not allowed.
+extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+void f2() {
+  extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+}
+
+// A block with variadic argument is not allowed.
+typedef int (^const bl_t)(int, ...); // expected-error{{OpenCL does not allow variadic arguments}}
+
+// A block cannot be the return value of a function.
+typedef int (^bl_t)(void);
+bl_t f3(bl_t bl); // expected-error{{declaring function return value of type 'bl_t' (aka 'int (^const)(int, ...)') is not allowed}}
+
+struct bl_s {
+  int (^bl)(void); // expected-error {{the 'int (^)(void)' type cannot be used to declare a structure or union field}}
+};
+
+void f4() {
+  __block int a = 10; // expected-error {{the __block storage type is not permitted}}
+}

test/SemaOpenCL/invalid-kernel-parameters.cl

 
 typedef struct FooImage2D // expected-note{{within field of type 'FooImage2D' declared here}}
 {
-  image2d_t imageField; // expected-note{{field of illegal type 'image2d_t' declared here}}
+  // TODO: Clean up needed - we don't really need to check for image, event, etc
+  // as a note here any longer.
+  // They are diagnosed as an error for all struct fields (OpenCL v1.2 s6.9b,r).
+  image2d_t imageField; // expected-note{{field of illegal type 'image2d_t' declared here}} expected-error{{the 'image2d_t' type cannot be used to declare a structure or union field}}
 } FooImage2D;
 
 kernel void image_in_struct_arg(FooImage2D arg) { } // expected-error{{struct kernel parameters may not contain pointers}}

test/SemaOpenCL/sampler_t.cl

 
 constant sampler_t glb_smp = 5;
 
-void foo(sampler_t); 
+void foo(sampler_t);
+
+constant struct sampler_s {
+  sampler_t smp; // expected-error {{the 'sampler_t' type cannot be used to declare a structure or union field}}
+} sampler_str = {0};
 
 void kernel ker(sampler_t argsmp) {
   local sampler_t smp; // expected-error {{sampler type cannot be used with the __local and __global address space qualifiers}}