[PATCH] Error on illegal OpenCL kernel argument types

[Richard Smith]

On Mon, Jul 8, 2013 at 9:17 PM, Matt Arsenault
<Matthew.Arsenault at amd.com> wrote:
> ================
> Comment at: lib/Sema/SemaDecl.cpp:5927-5937
> @@ +5926,13 @@
> +
> +    if (const TypedefType *Typedef = dyn_cast<TypedefType>(PT)) {
> +      const IdentifierInfo *Identifier = Typedef->getDecl()->getIdentifier();
> +      StringRef Name = Identifier->getName();
> +
> +      if (Name == "size_t" ||
> +          Name == "ptrdiff_t" ||
> +          Name == "intptr_t" ||
> +          Name == "uintptr_t") {
> +        return false;
> +      }
> +    }
> +  }
> ----------------
> Richard Smith wrote:
>> OpenCL 6.1.1 seems to imply that size_t, ptrdiff_t, intptr_t and uintptr_t are distinct built-in types in OpenCL, rather than being typedefs for some standard integer type, and it's the underlying types that we should be checking for here, not the typedefs. "typedef size_t my_size_t;" should also be prohibited here (as should, presumably, a typedef for __typeof(sizeof(0))).
>>
>> However, the OpenCL spec is unclear, so I'm somewhat guessing... :(
> I wasn't really sure what to do about these. I didn't see an equivalent in the builtin type enums I found. It sort of depends on the library implementation. Apple and libclc at least implicitly include a header before the program which has these typedefs in them.
>
> For example, the Apple header defines them as
> typedef __typeof__(((int*)0)-((int*)0)) ptrdiff_t;
> typedef __SIZE_TYPE__ size_t;
> typedef __SIZE_TYPE__ uintptr_t;
> typedef __PTRDIFF_TYPE__ intptr_t;
> typedef size_t       event_t;
>
> I don't actually see them defined in libclc, but size_t is used so I have no idea where that is coming from.

Maybe stddef.h is somehow included.

The SPIR specification implies pretty heavily that size_t is *not* a
typedef to some integer type with a known size (because it's
represented differently from any integer type at the IR level), and
the restriction here seems intended to be a part of making that work,
so I don't think it makes sense to implement it until we actually have
support for size_t as a distinct type.

> ================
> Comment at: lib/Sema/SemaDecl.cpp:5989-5990
> @@ +5988,4 @@
> +    const RecordDecl *RD = RT->getDecl();
> +    for (RecordDecl::field_iterator I = RD->field_begin(),
> +           E = RD->field_end(); I != E; ++I) {
> +      const FieldDecl *FD = *I;
> ----------------
> Richard Smith wrote:
>> Have you considered caching the result of this check? Or at least, storing a set of the types for which you've performed the check -- we don't really need to diagnose the same type more than once.
> The type itself isn't the problem, it's only when used as a kernel argument. I don't think only diagnosing it on the first parameter found and then not on a second use as another argument would make much sense.

If you want to diagnose the same type more than once, then please at
least keep track of those types which you've checked and found to be
OK. If a type is used as a subobject of a kernel argument once, it's
likely to be used as a subobject of a kernel argument again, and
you're just wasting cycles by checking it again.

Also, bail out once you've produced a diagnostic for a parameter;
there's no point producing more than one of those.