[cfe-dev] [PATCH] OpenCL 1.2 storage class specifiers and errors

[Tanya Lattner]

On Jun 14, 2012, at 3:58 AM, Anton Lokhmotov wrote:

> Hi Tanya (also copying cfe-dev for what may require a wider discussion),
> 
>> On Jun 13, 2012, at 1:31 PM, Peter Collingbourne wrote:
>> 
>>> We should respect the -cl-std option and only allow these storage class
>>> specifiers if -cl-std=CL1.2.
>> 
>> I don't necessarily disagree, but this hasn't been done for anything
>> regarding 1.1, so why start with 1.2? Does anyone really care about pre
>> OpenCL-1.2 since Clang didn't really support it fully?
> 
> I agree with Peter.  I'm sure that quite a few OpenCL implementers who use
> Clang have made sure it fully supported OpenCL 1.1 with their private
> patches.

Yes, but mainline Clang does not fully support OpenCL 1.0 or later. I am not saying that it should never be added, but I am not the one who has the time to add it at this point. If someone wants to go back and go through all the OpenCL changes and clarify which ones are 1.0, 1.1, 1.2 then that can be done in a separate step. I would rather not have my patches delayed just because we now have decided to force people to clarify what is 1.0, 1.1 and 1.2.

> 
> I do not suggest to go all the way back to OpenCL 1.0 and reject, say,
> three-element vectors, but support for OpenCL 1.1 is warranted.
> 
> We should probably introduce new language options for distinguishing between
> different OpenCL versions, e.g. OpenCL_1_1 (to mimic CL_VERSION_1_1 in the
> spec).
> 
> To make sure that comments unambiguously point to a version and section of
> the OpenCL specification, I propose to use the "OpenCL v<version number> s<
> section number>" format, e.g. "OpenCL v1.2 s6.8".
> 

Thats easily done.

> Please see couple of further comments below.

Thank you for your comments.

-Tanya

> 
> Cheers,
> Anton.
> 
> 
> +def err_static_kernel : Error<
> +  "kernel functions cannot be declared static">;
> Please make OpenCL-specific errors start with err_opencl: 
> err_opencl_static_kernel
> 
> +def err_static_local_scope : Error<
> +  "variables in local scope cannot be declared static">;
> I suggest to use function scope, as local has a different meaning in OpenCL:
> "function scope variables cannot be declared static"
> 
> 
> +  // OpenCL: Sec 6.8 -- The static qualifier is valid only in program
> +  // scope.
> +  if (getLangOpts().OpenCL) {
> +    if (NewVD->isStaticLocal()) {
> +      Diag(NewVD->getLocation(), diag::err_static_local_scope);
> +      NewVD->setInvalidDecl();
> +      return false;
> +    }
> +  }
> +
> 
> You can remove one level of nesting by using the && operator:
> 
> // OpenCL v1.2 s6.8: The static storage-class specifier can only 
> // be used for program scope variables.
> if (getLangOpts().OpenCL_1_2 && NewVD->isStaticLocal()) {
>  Diag(NewVD->getLocation(), diag::err_opencl_static_function_scope);
>  NewVD->setInvalidDecl();
>  return false;
> }
> 
> 
> +  if (getLangOpts().OpenCL) {
> +    if (NewFD->hasAttr<OpenCLKernelAttr>())
> +      // OpenCL: 6.8 static is invalid for kernel functions.
> +        if (SC == SC_Static) {
> +          Diag(D.getIdentifierLoc(), diag::err_static_kernel);
> +          D.setInvalidType();
> +        }
> +  }
> +
> 
> I think the second check is more expensive then the third one, so they
> should be swapped:
> 
> // OpenCL v1.2 s6.8: The static storage-class specifier can only 
> // be used for non-kernel functions.
> if (getLangOpts().OpenCL_1_2 && SC == SC_Static &&
>    NewFD->hasAttr<OpenCLKernelAttr>()) {
>  Diag(D.getIdentifierLoc(), diag::err_opencl_static_kernel);
>  D.setInvalidType();
> }
> 
> 
>