[PATCH] OpenCL 1.2 storage class specifiers and errors

Clang Frontend
#1

Hi Tanya (also copying cfe-dev for what may require a wider discussion),

> 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.

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".

Please see couple of further comments below.

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;
+ }
+ }

#2

Hi Anton,
I agree with you about the need to use the -cl-std option to decide if some features are supported, or if clang should throw an error.
Anyhow, I'd prefer to define the OpenCL version to be represented as an unsigned integer, similarly to what the OpenCL spec defines about the version macros: OpenCL 1.1 should be represented as 110, 1.2 as 120, etc...

This is useful, since features are supported beginning from some OpenCL version, so the right condition should be something like:

// OpenCL v1.2 s6.8: The static storage-class specifier can only
// be used for non-kernel functions.
if (getLangOpts().OpenCL_Version >= 120 && SC == SC_Static &&
    NewFD->hasAttr<OpenCLKernelAttr>()) {
  Diag(D.getIdentifierLoc(), diag::err_opencl_static_kernel);
  D.setInvalidType();
}

instead of going for something like

if (getLangOpts().OpenCL_1_2 || getLangOpts().OpenCL_2_0 ||...

thanks
    Guy