[cfe-commits] [PATCH] Error checking for OpenCL kernel return type and pointer args

[Anton Lokhmotov]

Hi Tanya,

I haven't seen this committed, so let me suggest slightly shorter error
messages:

+def err_expected_kernel_void_return_type : Error<
+  "a function using the __kernel qualifier must return type void">;

"kernel function must return void"

+def err_kernel_arg_with_private_address_space : Error<
+  "kernel pointer arguments must have a global, local, or constant address
space qualifier">;

"kernel argument cannot point to private address space"

Thanks,
Anton.


> Date: Fri, 24 Aug 2012 18:08:08 -0700
> From: Eli Friedman <eli.friedman at gmail.com>
> To: Tanya Lattner <lattner at apple.com>
> Cc: llvm cfe <cfe-commits at cs.uiuc.edu>
> Subject: Re: [cfe-commits] [PATCH] Error checking for OpenCL kernel
> 	return type and pointer args.
> Message-ID:
> 	<CAJdarcEhDK86_Qbw_bYndbddtwAz60VhEjs9erniteHVMTomLQ at mail.gmail.c
> om>
> Content-Type: text/plain; charset=ISO-8859-1
> 
> (Resending; I forgot to CC cfe-commits.)
> 
> On Fri, Aug 24, 2012 at 6:06 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
> > On Wed, Aug 22, 2012 at 1:14 PM, Tanya Lattner <lattner at apple.com>
> wrote:
> >> Ping.
> >>
> >> On Aug 15, 2012, at 6:11 PM, Tanya Lattner wrote:
> >>
> >>> The attached patch add the following checks and errors:
> >>> 1) Checks that an OpenCL kernel returns void, otherwise throws
> error.
> >>> 2) Check that kernel arguments that are pointers are in the
> constant, global, or local address space, otherwise throws an error.
> >>>
> >>> I have included a test case.
> >>>
> >>> Please review.
> >
> > +    if ((getLangOpts().OpenCLVersion >= 120)
> > +        && (SC == SC_Static)) {
> > +      Diag(D.getIdentifierLoc(), diag::err_static_kernel);
> > +      D.setInvalidType();
> > +    }
> >
> > Not really part of this patch, but the OpenCLVersion check appears to
> > be redundant.
> >
> > +      const PointerType *PT = dyn_cast<PointerType>(T.getTypePtr());
> >
> > T->getAs<PointerType>().  dyn_cast will mess up in the case of
> > typedefs.  (A test for this case would be nice.)
> >
> > You might also want to add a test for the case where the parameter is
> > declared as an array; I think your patch will work as-is, but it's
> > worth checking.
> >
> > +      if (PT
> > +          && PT->getPointeeType().getAddressSpace() == 0) {
> >
> > Weird indentation.
> >
> > +    // OpenCL v1.1 s6.8j Kernels may only have void return type.
> > +    if (!NewFD->getResultType()->isVoidType()) {
> > +      Diag(D.getIdentifierLoc(),
> > +           diag::err_expected_kernel_void_return_type);
> > +      D.setInvalidType();
> > +    }
> >
> > As written, this allows "const void"; not sure if that's intentional.
> >
> > Otherwise, looks fine.
> 
> -Eli
>