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

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

#2

Tanya Lattner wrote:

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.

Oh no, this would be extremely unpragmatic. I specifically said that we
should not try to distinguish between support for OpenCL 1.0 and OpenCL 1.1,
as the former is pretty much deprecated. (I hesitate to say the same about
the latter, as I am still polishing it :).)

Given that your new patches are first supporting OpenCL 1.2, I think now is
the right time to reflect on this problem to avoid going through the painful
process of reclassification in the future.

Guy Benyei wrote:

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

This sounds good. Even if a feature appears in one version and disappears
in a subsequent one, one can always use ranges (e.g. V >= 110 && < V <=
120).

The question is which OpenCL version should the existing
getLangOpts().OpenCL correspond to by default. Since today no OpenCL 1.2
features are on ToT, we can declare 1.1 as default and 1.2 features should
be added in the way you propose. At some amicably agreed point in the
future we can switch the default, by marking all OpenCL 1.1 specific
restrictions as such.

Guy, do you think you could prepare a patch that adds OpenCL_Version to
LangOpts? Then Tanya would be able to update her patches to use it.

Cheers,
Anton.

#3

Alright, since this proved to take 5 minutes (go Clang), I just did it because I don't want to be held up since I have a bunch of stuff I'm trying to push out.

Here is the patch, please review:

OpenCLVersion.patch (2.03 KB)

#4

Alright, since this proved to take 5 minutes (go Clang), I just did it
because I don't want to be held up since I have a bunch of stuff I'm
trying to push out.

Nice, thanks.

One comment. To enable using OpenCLVersion as Guy suggested:

if (getLangOpts().OpenCLVersion >= 120 && SC == SC_Static &&

you need to ensure that Opts.OpenCLVersion is initialised even when
Opts.OpenCL is 0, e.g.:

  LangOpts.OpenCLVersion = 0;
  ...
  if (LangStd == LangStandard::lang_opencl) {

And a niggle in descriptions: "OpenCL Version" -> "OpenCL version"?

Thanks,
Anton.

#5

Alright, since this proved to take 5 minutes (go Clang), I just did it
because I don't want to be held up since I have a bunch of stuff I'm
trying to push out.

Nice, thanks.

One comment. To enable using OpenCLVersion as Guy suggested:

if (getLangOpts().OpenCLVersion >= 120 && SC == SC_Static &&

you need to ensure that Opts.OpenCLVersion is initialised even when
Opts.OpenCL is 0, e.g.:

LangOpts.OpenCLVersion = 0;
...
if (LangStd == LangStandard::lang_opencl) {

The default is 0, it should already be initialized to that regardless of what OpenCL is.

And a niggle in descriptions: "OpenCL Version" -> "OpenCL version"?

Ok.

-Tanya