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

[Benyei, Guy]

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


-----Original Message-----
From: cfe-dev-bounces at cs.uiuc.edu [mailto:cfe-dev-bounces at cs.uiuc.edu] On Behalf Of Anton Lokhmotov
Sent: Thursday, June 14, 2012 13:59
To: 'Tanya Lattner'
Cc: cfe-dev at cs.uiuc.edu
Subject: Re: [cfe-dev] [PATCH] OpenCL 1.2 storage class specifiers and errors

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




_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.