[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

8227

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

8227

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

8258

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

enum OpenCLParamType {
   ValidKernelParam,
   PtrPtrKernelParam,
   PtrKernelParam,
   InvalidAddrSpacePtrKernelParam,
   InvalidKernelParam,
   RecordKernelParam
 };
 
+static bool isOpenCLSizeDependentType(ASTContext &C, QualType Ty) {
+  // Size dependent types are just typedefs to normal integer types
+  // (e.g. unsigned long), so we cannot distinguish them from other typedefs to
+  // integers other than by their names.
+  StringRef SizeTypeNames[] = {"size_t", "intptr_t", "uintptr_t", "ptrdiff_t"};
+
+  // Remove typedefs one by one until we reach a typedef
+  // for a size dependent type.
+  QualType DesugaredTy = Ty;
+  do {
+    ArrayRef<StringRef> Names(SizeTypeNames);
+    auto Match =
+        std::find(Names.begin(), Names.end(), DesugaredTy.getAsString());
+    if (Names.end() != Match)
+      return true;
+
+    Ty = DesugaredTy;
+    DesugaredTy = Ty.getSingleStepDesugaredType(C);
+  } while (DesugaredTy != Ty);
+
+  return false;
+}
+
 static OpenCLParamType getOpenCLKernelParameterType(Sema &S, QualType PT) {
   if (PT->isPointerType()) {
     QualType PointeeType = PT->getPointeeType();
     if (PointeeType->isPointerType())
       return PtrPtrKernelParam;
     if (PointeeType.getAddressSpace() == LangAS::opencl_generic ||
         PointeeType.getAddressSpace() == LangAS::opencl_private ||
         PointeeType.getAddressSpace() == LangAS::Default)
       return InvalidAddrSpacePtrKernelParam;
     return PtrKernelParam;
   }
 
-  // TODO: Forbid the other integer types (size_t, ptrdiff_t...) when they can
-  // be used as builtin types.
+  // OpenCL v1.2 s6.9.k:
+  // Arguments to kernel functions in a program cannot be declared with the
+  // built-in scalar types bool, half, size_t, ptrdiff_t, intptr_t, and
+  // uintptr_t or a struct and/or union that contain fields declared to be one
+  // of these built-in scalar types.
+  if (isOpenCLSizeDependentType(S.getASTContext(), PT))
+    return InvalidKernelParam;
 
   if (PT->isImageType())
     return PtrKernelParam;
 
   if (PT->isBooleanType() || PT->isEventT() || PT->isReserveIDT())
     return InvalidKernelParam;
 
   // OpenCL extension spec v1.2 s9.5:
case InvalidAddrSpacePtrKernelParam:
     // one of these built-in scalar types.
 
   case InvalidKernelParam:
     // OpenCL v1.2 s6.8 n:
     // A kernel function argument cannot be declared
     // of event_t type.
     // Do not diagnose half type since it is diagnosed as invalid argument
     // type for any function elsewhere.
-    if (!PT->isHalfType())
+    if (!PT->isHalfType()) {
       S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
+
+      // Explain what typedefs are involved.
+      const TypedefType *Typedef = nullptr;
+      while ((Typedef = PT->getAs<TypedefType>())) {
+        SourceLocation Loc = Typedef->getDecl()->getLocation();
+        // SourceLocation may be invalid for a built-in type.
+        if (Loc.isValid())
+          S.Diag(Loc, diag::note_entity_declared_at) << PT;
+        PT = Typedef->desugar();
+      }
+    }
+
     D.setInvalidType();
     return;
 
   case PtrKernelParam:
   case ValidKernelParam:
     ValidTypes.insert(PT.getTypePtr());
     return;
 
do {
     // 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);
 
       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);
     }
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;

test/SemaOpenCL/invalid-kernel-parameters.cl

 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
 
 kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type 'half' is not allowed; did you forget * ?}}
 
 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 
 // Disallowed: parameters with type
 // bool, half, size_t, ptrdiff_t, intptr_t, and uintptr_t
 // or a struct / union with any of these types in them
 
-// TODO: Ban int types, size_t, ptrdiff_t ...
+typedef __SIZE_TYPE__ size_t; // expected-note{{'size_t' (aka 'unsigned int') declared here}}
+                              // expected-note@-1{{'size_t' (aka 'unsigned int') declared here}}
+typedef __PTRDIFF_TYPE__ ptrdiff_t; // expected-note{{'ptrdiff_t' (aka 'int') declared here}}
+typedef __INTPTR_TYPE__ intptr_t; // expected-note{{'intptr_t' (aka 'int') declared here}}
+typedef __UINTPTR_TYPE__ uintptr_t; // expected-note{{'uintptr_t' (aka 'unsigned int') declared here}}
+
+kernel void size_t_arg(size_t x) {} // expected-error{{'size_t' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
+
+kernel void ptrdiff_t_arg(ptrdiff_t x) {} // expected-error{{'ptrdiff_t' (aka 'int') cannot be used as the type of a kernel parameter}}
+
+kernel void intptr_t_arg(intptr_t x) {} // expected-error{{'intptr_t' (aka 'int') cannot be used as the type of a kernel parameter}}
+
+kernel void uintptr_t_arg(uintptr_t x) {} // expected-error{{'uintptr_t' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
+
+typedef size_t size_ty;
+struct SizeTStruct { // expected-note{{within field of type 'SizeTStruct' declared here}}
+  size_ty s; // expected-note{{field of illegal type 'size_ty' (aka 'unsigned int') declared here}}
+};
+kernel void size_t_struct_arg(struct SizeTStruct x) {} // expected-error{{'struct SizeTStruct' cannot be used as the type of a kernel parameter}}
+
+union SizeTUnion { // expected-note{{within field of type 'SizeTUnion' declared here}}
+  size_t s; // expected-note{{field of illegal type 'size_t' (aka 'unsigned int') declared here}}
+  float f;
+};
+kernel void size_t_union_arg(union SizeTUnion x) {} // expected-error{{'union SizeTUnion' cannot be used as the type of a kernel parameter}}
+
+typedef size_t s_ty; // expected-note{{'s_ty' (aka 'unsigned int') declared here}}
+typedef s_ty ss_ty; // expected-note{{'ss_ty' (aka 'unsigned int') declared here}}
+kernel void typedef_to_size_t(ss_ty s) {} // expected-error{{'ss_ty' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
 
 kernel void bool_arg(bool x) { } // expected-error{{'bool' cannot be used as the type of a kernel parameter}}
 
 // half kernel argument is allowed when cl_khr_fp16 is enabled.
 kernel void half_arg(half x) { }
 
 typedef struct ContainsBool // expected-note{{within field of type 'ContainsBool' declared here}}
 {