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

[Anton Lokhmotov]

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.

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

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();
}