[OpenCL] Check for invalid kernel arguments in array types

[asavonic]

Summary

OpenCL specification forbids use of several types as kernel
arguments. This patch improves existing diagnostic to look through
arrays.

[Anastasia]

LGTM! Thanks!

[yaxunl]

LGTM

[Anastasia]

Btw, has this restriction been removed from CL 2.0?

[asavonic]

Moved a chunk from https://reviews.llvm.org/D49725; added 2 more tests.

[asavonic]

In D49723#1173352, @Anastasia wrote:

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

[asavonic]

Moved another chunk from https://reviews.llvm.org/D49725

[Anastasia]

In D49723#1174837, @asavonic wrote:

In D49723#1173352, @Anastasia wrote:

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

lib/Sema/SemaDecl.cpp

8187

Do we need to assert PT here too? It doesn't seem to be modified in this loop...

[asavonic]

Fix assert.

[asavonic]

In D49723#1178127, @Anastasia wrote:

In D49723#1174837, @asavonic wrote:

In D49723#1173352, @Anastasia wrote:

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

Right, and it seems that pointers in struct arguments should also be legal in CL2.0.
I'll submit another patch to remove this check for CL2.0.

lib/Sema/SemaDecl.cpp

8187

We should check for a field instead. Thanks!

[asavonic]

Rollback accidentally squashed commit.

[Anastasia]

LGTM! Thanks!

[yaxunl]

LGTM. Thanks.

[Ayal]

In D49723#1178186, @asavonic wrote:

In D49723#1178127, @Anastasia wrote:

In D49723#1174837, @asavonic wrote:

In D49723#1173352, @Anastasia wrote:

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

Right, and it seems that pointers in struct arguments should also be legal in CL2.0.
I'll submit another patch to remove this check for CL2.0.

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

[asavonic]

In D49723#4116435, @Ayal wrote:

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

Yeah, the patch got lost somewhere, I'm sorry...

TBH, I don't know why pointers in structs or arrays were disallowed in the first place. Even OpenCL 1.2 does not say it explicitly, although there is a bit suspicious point in s6.9.p:

Arguments to kernel functions that are declared to be a struct or union do not allow
OpenCL objects to be passed as elements of the struct or union

It does not say "pointers", just "OpenCL objects". Sounds more like events or images to me, not pointers.

[Ayal]

In D49723#4116783, @asavonic wrote:

In D49723#4116435, @Ayal wrote:

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

Yeah, the patch got lost somewhere, I'm sorry...

Sure, trying to resurrect it in a comment above, and in D143849.

TBH, I don't know why pointers in structs or arrays were disallowed in the first place. Even OpenCL 1.2 does not say it explicitly, although there is a bit suspicious point in s6.9.p:

Arguments to kernel functions that are declared to be a struct or union do not allow
OpenCL objects to be passed as elements of the struct or union

It does not say "pointers", just "OpenCL objects". Sounds more like events or images to me, not pointers.

Yeah, there's room to improve clarity of the spec. Passing pointers indirectly requires them to point to same/share address space, as provided by SVM starting in OpenCL2.0.

lib/Sema/SemaDecl.cpp

8218

Suggest to (rebase and) update this as follows, based on
https://registry.khronos.org/OpenCL/sdk/2.0/docs/man/xhtml/restrictions.html

// union. This restriction was lifted in OpenCL v2.0 with the introduction
// of SVM.
if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
    (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
  continue;

lib/Sema/SemaDecl.cpp

static OpenCLParamType getOpenCLKernelParameterType(Sema &S, QualType PT) {
   // This extension adds support for half scalar and vector types as built-in
   // types that can be used for arithmetic operations, conversions etc.
   if (!S.getOpenCLOptions().isEnabled("cl_khr_fp16") && PT->isHalfType())
     return InvalidKernelParam;
 
   if (PT->isRecordType())
     return RecordKernelParam;
 
+  // Look into an array argument to check if it has a forbidden type.
+  if (PT->isArrayType()) {
+    const Type *UnderlyingTy = PT->getPointeeOrArrayElementType();
+    // Call ourself to check an underlying type of an array. Since the
+    // getPointeeOrArrayElementType returns an innermost type which is not an
+    // array, this recusive call only happens once.
+    return getOpenCLKernelParameterType(S, QualType(UnderlyingTy, 0));
+  }
+
   return ValidKernelParam;
 }
 
 static void checkIsValidOpenCLKernelParameter(
   Sema &S,
   Declarator &D,
   ParmVarDecl *Param,
   llvm::SmallPtrSetImpl<const Type *> &ValidTypes) {
static void checkIsValidOpenCLKernelParameter(
   // Track nested structs we will inspect
   SmallVector<const Decl *, 4> VisitStack;
 
   // Track where we are in the nested structs. Items will migrate from
   // VisitStack to HistoryStack as we do the DFS for bad field.
   SmallVector<const FieldDecl *, 4> HistoryStack;
   HistoryStack.push_back(nullptr);
 
-  const RecordDecl *PD = PT->castAs<RecordType>()->getDecl();
-  VisitStack.push_back(PD);
+  // At this point we already handled everything except of a RecordType or
+  // an ArrayType of a RecordType.
+  assert((PT->isArrayType() || PT->isRecordType()) && "Unexpected type.");
+  const RecordType *RecTy =
+      PT->getPointeeOrArrayElementType()->getAs<RecordType>();
+  const RecordDecl *OrigRecDecl = RecTy->getDecl();
 
+  VisitStack.push_back(RecTy->getDecl());
   assert(VisitStack.back() && "First decl null?");
 
   do {
     const Decl *Next = VisitStack.pop_back_val();
     if (!Next) {
       assert(!HistoryStack.empty());
       // Found a marker, we have gone up a level
       if (const FieldDecl *Hist = HistoryStack.pop_back_val())
         ValidTypes.insert(Hist->getType().getTypePtr());
 
       continue;
     }
 
     // Adds everything except the original parameter declaration (which is not a
     // field itself) to the history stack.
     const RecordDecl *RD;
     if (const FieldDecl *Field = dyn_cast<FieldDecl>(Next)) {
       HistoryStack.push_back(Field);
-      RD = Field->getType()->castAs<RecordType>()->getDecl();
+
+      QualType FieldTy = Field->getType();
+      // Other field types (known to be valid or invalid) are handled while we
+      // walk around RecordDecl::fields().
+      assert((FieldTy->isArrayType() || FieldTy->isRecordType()) &&
+             "Unexpected type.");
+      const Type *FieldRecTy = FieldTy->getPointeeOrArrayElementType();
+
+      RD = FieldRecTy->castAs<RecordType>()->getDecl();
     } else {
       RD = cast<RecordDecl>(Next);
     }
 
     // Add a null marker so we know when we've gone back up a level
     VisitStack.push_back(nullptr);
 
     for (const auto *FD : RD->fields()) {
for (const auto *FD : RD->fields()) {
       if (ParamType == RecordKernelParam) {
         VisitStack.push_back(FD);
         continue;
       }
 
       // OpenCL v1.2 s6.9.p:
       // Arguments to kernel functions that are declared to be a struct or union
       // do not allow OpenCL objects to be passed as elements of the struct or
       // union.
       if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam ||
           ParamType == InvalidAddrSpacePtrKernelParam) {
         S.Diag(Param->getLocation(),
                diag::err_record_with_pointers_kernel_param)
           << PT->isUnionType()
           << PT;
       } else {
         S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
       }
 
-      S.Diag(PD->getLocation(), diag::note_within_field_of_type)
-        << PD->getDeclName();
+      S.Diag(OrigRecDecl->getLocation(), diag::note_within_field_of_type)
+          << OrigRecDecl->getDeclName();
 
       // We have an error, now let's go back up through history and show where
       // the offending field came from
       for (ArrayRef<const FieldDecl *>::const_iterator
                I = HistoryStack.begin() + 1,
                E = HistoryStack.end();
            I != E; ++I) {
         const FieldDecl *OuterField = *I;

test/SemaOpenCL/invalid-kernel-parameters.cl

 {
   float x;
   struct Valid valid1;
   struct Valid valid2;
   struct NestedPointer aaaa; // expected-note{{within field of type 'struct NestedPointer' declared here}}
 };
 
 kernel void pointer_in_nested_struct_arg_2(struct Valid valid, struct NestedPointer arg, struct AlsoUser also) { } // expected-error 2 {{struct kernel parameters may not contain pointers}}
+
+struct ArrayOfPtr // expected-note{{within field of type 'ArrayOfPtr' declared here}}
+{
+  float *arr[3]; // expected-note{{field of illegal type 'float *[3]' declared here}}
+                 // expected-note@-1{{field of illegal type 'float *[3]' declared here}}
+};
+kernel void array_of_ptr(struct ArrayOfPtr arr) {} // expected-error{{struct kernel parameters may not contain pointers}}
+
+struct ArrayOfStruct // expected-note{{within field of type 'ArrayOfStruct' declared here}}
+{
+  struct ArrayOfPtr arr[3]; // expected-note{{within field of type 'struct ArrayOfPtr [3]' declared here}}
+};
+kernel void array_of_struct(struct ArrayOfStruct arr) {} // expected-error{{struct kernel parameters may not contain pointers}}