Page MenuHomePhabricator

[OpenCL] Postpone PSV address space diagnostic
Needs ReviewPublic

Authored by svenvh on Oct 25 2018, 8:26 AM.

Details

Summary

In OpenCL C++ mode, the "program scope variable must reside ..."
diagnostic is firing early in some cases, potentially confusing the
user. It is the first diagnostic reported for

int g(global int *p) {}

but the actual problem is the undeclared identifier global.

The main reason to improve error reporting for the example above is
that OpenCL C address space qualifiers such as global are not part
of OpenCL C++. This means the PSV diagnostic will fire when porting
OpenCL C code to OpenCL C++ if an address space qualifier is left in
by accident.

The PSV diagnostic is emitted for the example above because the parser
believes g is a variable declaration instead of a function
declaration. This seems to be inherent to C++ parsing, so postpone
the PSV diagnostic until we have parsed the entire declarator group.
We still get the PSV diagnostic for the example, but it is no longer
the first so it should be easier for the user to see what the actual
problem is.

Diff Detail

Event Timeline

svenvh created this revision.Oct 25 2018, 8:26 AM

I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

So OpenCL C code has to be completely rewritten if it needs to be used as part of an OpenCL C++ program? And it doesn't really compose like a type if it's supposed to change how a variable is stored. What a terrible little mess they've made for themselves.

I think a better solution might be to hack the parser to recognize cases like this. It's more annoying work, but it'll lead to much better results.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

So OpenCL C code has to be completely rewritten if it needs to be used as part of an OpenCL C++ program? And it doesn't really compose like a type if it's supposed to change how a variable is stored. What a terrible little mess they've made for themselves.

Fair point. I would like to allow regular OpenCL address space qualifiers as a Clang feature at least, in case we won't be able to alter the spec. But one problem is that the private address space qualifier keyword conflicts with the private class access modifier. I guess we can only allow the address space qualifiers with the __ prefix. So some existing code would have to be changed when ported to OpenCL C++, but it should be easier than rewriting using classes.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

So OpenCL C code has to be completely rewritten if it needs to be used as part of an OpenCL C++ program? And it doesn't really compose like a type if it's supposed to change how a variable is stored. What a terrible little mess they've made for themselves.

Fair point. I would like to allow regular OpenCL address space qualifiers as a Clang feature at least, in case we won't be able to alter the spec. But one problem is that the private address space qualifier keyword conflicts with the private class access modifier. I guess we can only allow the address space qualifiers with the __ prefix. So some existing code would have to be changed when ported to OpenCL C++, but it should be easier than rewriting using classes.

That's actually a really easy ambiguity to handle given the current uses of the access-modifier keywords. You just don't parse private as an access modifier if it's not followed by a colon.

We could submit to the standard an OpenCL C++ extension to accept the OpenCL C syntax...

I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is.

You are actually telling this to Khronos since a lot of Khronos members are on the LLVM mailing lists and are reading this right now... :-)
I am unsure to understand what you mean about "the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is".
The motivation behind this was to have 0 language extension to C++ in OpenCL C++, like with the single-source version of the standard, SYCL, where you can have a program which is executed purely on CPU without any compiler support, to avoid the CUDA & C++AMP atrocities.
That seems like a good discussion topic next Thursday at the LLVM Bay Area Meetup. :-)

I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is.

You are actually telling this to Khronos since a lot of Khronos members are on the LLVM mailing lists and are reading this right now... :-)

Great.

I am unsure to understand what you mean about "the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is".

New versions of C++ have semi-regularly added keywords like static_assert and thread_local that are not in the reserved space for identifiers. When C adopted these, it spelled them _Static_assert and _Thread_local, which are in the reserved space. I was under the mistaken (right?) impression thought that e.g. __constant was a standardized spelling, and I thought that Khronos had just decided to avoid using the unreserved identifiers in C++ when it had gladly adopted them in C. But now I understand that OpenCL C++ is attempting to be a radically different language model that does not actually have qualifiers at all.

The motivation behind this was to have 0 language extension to C++ in OpenCL C++, like with the single-source version of the standard, SYCL, where you can have a program which is executed purely on CPU without any compiler support, to avoid the CUDA & C++AMP atrocities.

By "0 language extension", do you just mean no *syntactic* extensions? Because something like cl::priv certainly seems like it requires intrinsic language support, and I don't know what add_global_t could possibly do besides add a not-described-in-the-standard-but-still-obviously-there address-space qualifier so that member accesses continue to work and properly preserve the qualifier on the resulting l-value. C++ doesn't provide adequate language tools to replace pointers and references at a language level, not by a long shot; the tools are only really good enough for resource management.

The "just compile C++ code for the GPU" idea that (IIRC) AMD has been exploring seems quite interesting, but unlike OpenCL C++ it really is a largely no-extensions approach: it's implementing the classic C++ language model (with facilities for taking more control) like any other frontend, just with a weird target on the backend. It's also just hoping that the optimizer will be able to eliminate the overheads when mapping to a multiple-address-space model, and I'm not sure whether that's been borne out. OpenCL C++ seems to be trying to strike a middle ground where there are minimal syntactic extensions but the type system is rather radically different in ways that often work but also often don't, and it's basically held together by the underlying extensions that it's pretending aren't there. That doesn't seem like a successful language approach.

Also, I have no opinion about CUDA or C++AMP but feel I need to distance myself from your comment about them.

That seems like a good discussion topic next Thursday at the LLVM Bay Area Meetup. :-)

Alas, I live in New York; I was just out there for the Developer's Meeting but won't be back for some time.

FYI, I have created a bug to the OpenCL C++ spec: https://github.com/KhronosGroup/OpenCL-Docs/issues/13.

Gladly, we now moved into discussing the issues publicly. So anyone can create bugs and also participate in discussions.

I would like to feed all of the suggestions back and try to fix problematic parts of the language as much as possible.

New versions of C++ have semi-regularly added keywords like static_assert and thread_local that are not in the reserved space for identifiers. When C adopted these, it spelled them _Static_assert and _Thread_local, which are in the reserved space. I was under the mistaken (right?) impression thought that e.g. __constant was a standardized spelling, and I thought that Khronos had just decided to avoid using the unreserved identifiers in C++ when it had gladly adopted them in C.

Thank you for the clarification.

But now I understand that OpenCL C++ is attempting to be a radically different language model that does not actually have qualifiers at all.

Yes.

By "0 language extension", do you just mean no *syntactic* extensions? Because something like cl::priv certainly seems like it requires intrinsic language support, and I don't know what add_global_t could possibly do besides add a not-described-in-the-standard-but-still-obviously-there address-space qualifier so that member accesses continue to work and properly preserve the qualifier on the resulting l-value. C++ doesn't provide adequate language tools to replace pointers and references at a language level, not by a long shot; the tools are only really good enough for resource management.

Since it is C++, you can do a lot with some wrapper classes and so not requiring visible language extensions.
But yes, if you dig into the implementation, it will use some attributes, address-spaces and so on.

The "just compile C++ code for the GPU" idea that (IIRC) AMD has been exploring seems quite interesting, but unlike OpenCL C++ it really is a largely no-extensions approach: it's implementing the classic C++ language model (with facilities for taking more control) like any other frontend, just with a weird target on the backend. It's also just hoping that the optimizer will be able to eliminate the overheads when mapping to a multiple-address-space model, and I'm not sure whether that's been borne out. OpenCL C++ seems to be trying to strike a middle ground where there are minimal syntactic extensions but the type system is rather radically different in ways that often work but also often don't, and it's basically held together by the underlying extensions that it's pretending aren't there. That doesn't seem like a successful language approach.

I was really meaning "run on the CPU" and not "run on the GPU", it was not a typo from me. :-)
If there are no visible language extensions besides C++ classes, it is easier to run the kernel code on a CPU without any specific compiler support, with just a plain C++ compiler and just by changing the runtime to launch the kernel.
It was part of the design motivation to remove the alien keywords.
This is even more true with SYCL.

That seems like a good discussion topic next Thursday at the LLVM Bay Area Meetup. :-)

Alas, I live in New York; I was just out there for the Developer's Meeting but won't be back for some time.

We will miss you, then. :-)

I was really meaning "run on the CPU" and not "run on the GPU", it was not a typo from me. :-)
If there are no visible language extensions besides C++ classes, it is easier to run the kernel code on a CPU without any specific compiler support, with just a plain C++ compiler and just by changing the runtime to launch the kernel.
It was part of the design motivation to remove the alien keywords.
This is even more true with SYCL.

Okay. So the purpose of OpenCL C++ is to provide a non-unified model that allows you to easily run C++ code on the CPU.

Okay. So the purpose of OpenCL C++ is to provide a non-unified model that allows you to easily run C++ code on the CPU.

It is just the second-order purpose.
A C++-based kernel language to program accelerators without C++ language extension, so that the code could also mostly run on a CPU without a specific compiler.

Okay. So the purpose of OpenCL C++ is to provide a non-unified model that allows you to easily run C++ code on the CPU.

It is just the second-order purpose.
A C++-based kernel language to program accelerators without C++ language extension, so that the code could also mostly run on a CPU without a specific compiler.

So, as a C++ language expert, I don't think this is actually possible without major compromises (e.g. preventing users from using structs in non-generic address spaces), and I think you're being somewhat willfully blind about the extent to which you are going to be relying on language extensions — in ways that are unavoidably exposed in the language, and which therefore can change the well-formedness or dynamic behavior of the program — to make this work on a GPU (or any other device with interesting address spaces). If your plan is to compile code for the CPU with a normal C++ compiler but for the GPU with an OpenCL-aware compiler, you are going to have significantly divergent behavior between the CPU and GPU language dialects because of the non-standardness of the GPU dialect.

If your plan is to compile code for the CPU with a normal C++ compiler but for the GPU with an OpenCL-aware compiler, you are going to have significantly divergent behavior between the CPU and GPU language dialects because of the non-standardness of the GPU dialect.

We are trying to minimize the divergence. Thus the use of conditional tense and *mostly" word in the sentence "could mostly run on a CPU without a specific compiler", as a wishful thinking design trade-off, trying to minimize the rewriting a user has to do starting from a plain C++ function.

Probably we can discuss this next week off-line during a ISO C++ SG14 or SG1 session...

I'm just concerned about adding a great deal of complexity to mainline Clang in order to support a language that might still be in major flux and which I feel is likely to be forced to re-embrace qualifiers in the language model. Maybe this work should happen in a branch for awhile, at least for superficial things — patches to e.g. generalize things at the IRGen level to handle address spaces in various C++ language features would still be welcome, since those are likely to be useful across language modes.