[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

7700

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

7717

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

13102–13106

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

7

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

7

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

def err_parameters_retval_cannot_have_fp16_type : Error<
   "%select{parameters|function return value}0 cannot have __fp16 type; did you forget * ?">;
 def err_opencl_half_load_store : Error<
   "%select{loading directly from|assigning directly to}0 pointer to type %1 is not allowed">;
 def err_opencl_cast_to_half : Error<"casting to type %0 is not allowed">;
 def err_opencl_half_declaration : Error<
   "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">,
   InGroup<IgnoredPragmas>;
 def err_pragma_options_align_mac68k_target_unsupported : Error<
   "mac68k alignment pragma is not supported on this target">;
 def warn_pragma_pack_invalid_alignment : Warning<
   "expected #pragma pack parameter to be '1', '2', '4', '8', or '16'">,

 def err_record_with_pointers_kernel_param : Error<
   "%select{struct|union}0 kernel parameters may not contain pointers">;
 def note_within_field_of_type : Note<
   "within field of type %0 declared here">;
 def note_illegal_field_declared_here : Note<
   "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<
   "kernel must have void return type">;
 def err_sampler_argument_required : Error<
   "sampler_t variable required - got %0">;
 def err_wrong_sampler_addressspace: Error<
   "sampler type cannot be used with the __local and __global address space qualifiers">;
def err_opencl_return_value_with_address_space : Error<
   "return value cannot be qualified with address space">;
 def err_opencl_constant_no_init : Error<
   "variable in constant address space must be initialized">;
 def err_atomic_init_constant : Error<
   "atomic variable can only be assigned to a compile time constant"
   " in the declaration statement in the program scope">;
 def err_opencl_implicit_vector_conversion : Error<
   "implicit conversions between vector types (%0 and %1) are not permitted">;
+def err_opencl_variadic_prototype_not_allowed : Error<
+  "OpenCL does not allow variadic arguments">;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<
   "first argument to %0 must be a pipe type">;
 def err_opencl_builtin_pipe_arg_num : Error<
     "invalid number of arguments to function: %0">;
 def err_opencl_builtin_pipe_invalid_arg : Error<
   "invalid argument type to function %0 (expecting %1)">;
 def err_opencl_builtin_pipe_invalid_access_modifier : Error<
   "invalid pipe access modifier (expecting %0)">;
 
 // OpenCL Section 6.8.g
 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 {
 // OpenMP support.
 def err_omp_expected_var_arg : Error<
   "%0 is not a global variable, static local variable or static data member">;
 def err_omp_expected_var_arg_suggest : Error<
   "%0 is not a global variable, static local variable or static data member; "

lib/Sema/SemaDecl.cpp

void Sema::CheckVariableDeclarationType(VarDecl *NewVD) {
   // ISO/IEC TR 18037 S5.1.2
   if (!getLangOpts().OpenCL
       && NewVD->hasLocalStorage() && T.getAddressSpace() != 0) {
     Diag(NewVD->getLocation(), diag::err_as_qualified_auto_decl);
     NewVD->setInvalidDecl();
     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 &&
       NewVD->isStaticLocal()) {
     Diag(NewVD->getLocation(), diag::err_static_function_scope);
     NewVD->setInvalidDecl();
     return;
   }
 
+  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 (getLangOpts().OpenCL) {
     if (NewVD->isFileVarDecl()) {
       if (!T->isSamplerT() &&
           !(T.getAddressSpace() == LangAS::opencl_constant ||
             (T.getAddressSpace() == LangAS::opencl_global &&
              getLangOpts().OpenCLVersion == 200))) {
         if (getLangOpts().OpenCLVersion == 200)
           Diag(NewVD->getLocation(), diag::err_opencl_global_invalid_addr_space)
               << "global or constant";
Sema::ActOnCXXForRangeIdentifier(Scope *S, SourceLocation IdentLoc,
   FinalizeDeclaration(Var);
   return ActOnDeclStmt(FinalizeDeclaratorGroup(S, DS, Var), IdentLoc,
                        AttrEnd.isValid() ? AttrEnd : IdentLoc);
 }
 
 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 &&
       var->hasLocalStorage()) {
     switch (var->getType().getObjCLifetime()) {
     case Qualifiers::OCL_None:
     case Qualifiers::OCL_ExplicitNone:
     case Qualifiers::OCL_Autoreleasing:
FieldDecl *Sema::HandleField(Scope *S, RecordDecl *Record,
   }
 
   // TR 18037 does not allow fields to be declared with address spaces.
   if (T.getQualifiers().hasAddressSpace()) {
     Diag(Loc, diag::err_field_with_address_space);
     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();
   }
 
   DiagnoseFunctionSpecifiers(D.getDeclSpec());
 
   if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
     Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
          diag::err_invalid_thread)

lib/Sema/SemaType.cpp

case DeclaratorChunk::Function: {
         D.setInvalidType(true);
       }
 
       // Do not allow returning half FP value.
       // FIXME: This really should be in BuildFunctionType.
       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) {
           S.Diag(D.getIdentifierLoc(),
             diag::err_parameters_retval_cannot_have_fp16_type) << 1;
           D.setInvalidType(true);
         }
       }
 
+      // All OpenCL misc checks
+      if (LangOpts.OpenCL) {
+        // OpenCL v1.2 s6.9 - Variadic functions are not supported, except for
+        // printf
+        // OpenCL v2.0 s6.12.5 - Blocks with variadic arguments are not
+        // allowed
+        if (FTI.isVariadic) {
+          bool IsValidOCLPrintf = false;
+          if (D.getIdentifier()->getName() == "printf" && FTI.NumParams > 0) {
+            ParmVarDecl *Param = cast<ParmVarDecl>(FTI.Params[0].Param);
+            QualType ParamTy = Param->getType();
+            if (ParamTy->isPointerType() &&
+                ParamTy->getPointeeType().getAddressSpace() ==
+                    LangAS::opencl_constant &&
+                ParamTy->getPointeeType()->isCharType()) {
+              IsValidOCLPrintf = true;
+            }
+          }
+          if (!IsValidOCLPrintf) {
+            S.Diag(D.getIdentifierLoc(),
+                   diag::err_opencl_variadic_prototype_not_allowed);
+            D.setInvalidType(true);
+          }
+        }
+        // OpenCL v2.0 s6.12.5 - A block cannot be the return value of a
+        // function.
+        if (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()) {
         SourceLocation DiagLoc, FixitLoc;
         if (TInfo) {
           DiagLoc = TInfo->getTypeLoc().getLocStart();
           FixitLoc = S.getLocForEndOfToken(TInfo->getTypeLoc().getLocEnd());
         } else {

test/CodeGen/blocks-opencl.cl

 // RUN: %clang_cc1 -O0 %s -ffake-address-space-map -emit-llvm -o - -fblocks -triple x86_64-unknown-unknown | FileCheck %s
 // 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

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 
 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}}
 
 void kernel ker(event_t argevt) { // expected-error {{'event_t' cannot be used as the type of a kernel parameter}}
   event_t e;
   constant event_t const_evt; // expected-error {{the event_t type can only be used with __private address space qualifier}}
   foo(e);
   foo(0);
   foo(5); // expected-error {{passing 'int' to parameter of incompatible type 'event_t'}}
 }
 

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-func.cl

+// RUN: %clang_cc1 %s -verify -fsyntax-only -cl-std=CL2.0 -DCL20
+
+void foo(int, ...); // expected-error{{OpenCL does not allow variadic arguments}}
+int printf(constant const char *, ...);

test/SemaOpenCL/invalid-kernel-parameters.cl

 } ContainsBool;
 
 kernel void bool_in_struct_arg(ContainsBool x) { } // expected-error{{'ContainsBool' (aka 'struct ContainsBool') cannot be used as the type of a kernel parameter}}
 
 
 
 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}}
 
 typedef struct Foo // expected-note{{within field of type 'Foo' declared here}}
 {
   int* ptrField; // expected-note{{field of illegal pointer type 'int *' declared here}}
 } Foo;

test/SemaOpenCL/sampler_t.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 
 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}}
   const sampler_t const_smp = 7;
   foo(glb_smp);
   foo(const_smp);
   foo(5); // expected-error {{sampler_t variable required - got 'int'}}
 }