[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
Anastasia added a comment. Herald added a subscriber: ebevhan. Herald added a project: clang. Btw, I have created a patch that allows non-underscore prefixed spelling for address spaces in C++ for OpenCL: https://reviews.llvm.org/D59603 I assume the error becomes less critical however it remains. I think it still makes sense to fix it considering that it isn't very costly? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53705/new/ https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
keryell added a comment. In https://reviews.llvm.org/D53705#1285096, @rjmccall wrote: > > 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... Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. In https://reviews.llvm.org/D53705#1284734, @keryell wrote: > In https://reviews.llvm.org/D53705#1284268, @rjmccall wrote: > > > > > > > > > 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 `struct`s 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
keryell added a comment. In https://reviews.llvm.org/D53705#1284268, @rjmccall wrote: > > 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. In https://reviews.llvm.org/D53705#1284203, @keryell wrote: > 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
keryell added a comment. In https://reviews.llvm.org/D53705#1281743, @rjmccall wrote: > > 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. :-) Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
Anastasia added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. In https://reviews.llvm.org/D53705#1281738, @keryell wrote: > In https://reviews.llvm.org/D53705#1278066, @rjmccall wrote: > > > 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
keryell added a comment. In https://reviews.llvm.org/D53705#1278066, @rjmccall wrote: > 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. :-) Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
keryell added a comment. We could submit to the standard an OpenCL C++ extension to accept the OpenCL C syntax... Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. In https://reviews.llvm.org/D53705#1280264, @Anastasia wrote: > In https://reviews.llvm.org/D53705#1279086, @rjmccall wrote: > > > In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > > > > > 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` 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
Anastasia added a comment. In https://reviews.llvm.org/D53705#1279086, @rjmccall wrote: > In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > > > 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` 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > 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` 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
svenvh added a comment. 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` through a header file. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
rjmccall added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
svenvh created this revision.
svenvh added reviewers: Anastasia, rjmccall.
Herald added subscribers: cfe-commits, yaxunl.
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.
Repository:
rC Clang
https://reviews.llvm.org/D53705
Files:
lib/Sema/SemaDecl.cpp
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
test/SemaOpenCLCXX/invalid-qualifier.cl
Index: test/SemaOpenCLCXX/invalid-qualifier.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/invalid-qualifier.cl
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -fsyntax-only -verify
+// RUN: not %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -fsyntax-only 2>&1 | FileCheck %s
+
+// Use FileCheck to check that "program scope variable must reside ..." is not
+// the first diagnostic reported.
+
+int f1(global int *p) {}
+// expected-error@-1{{use of undeclared identifier 'global'}}
+// expected-error@-2{{expected ';' after top level declarator}}
+// expected-error@-3{{program scope variable must reside in constant address space}}
+// CHECK: use of undeclared identifier 'global'
+// CHECK: program scope variable must reside in constant address space
+
+; // For parser recovery.
+
+int f2(local int *p) {}
+// expected-error@-1{{use of undeclared identifier 'local'}}
+// expected-error@-2{{expected ';' after top level declarator}}
+// expected-error@-3{{program scope variable must reside in constant address space}}
+// CHECK: use of undeclared identifier 'local'
+// CHECK: program scope variable must reside in constant address space
+
+; // For parser recovery.
+
+int g(abcdef int q) {}
+// expected-error@-1{{use of undeclared identifier 'abcdef'}}
+// expected-error@-2{{expected ';' after top level declarator}}
+// expected-error@-3{{program scope variable must reside in constant address space}}
+// CHECK: use of undeclared identifier 'abcdef'
+// CHECK: program scope variable must reside in constant address space
+
+; // For parser recovery.
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -4,11 +4,12 @@
constant int G2 = 0;
int G3 = 0;// expected-error{{program scope variable must reside in constant address space}}
global int G4 = 0; // expected-error{{program scope variable must reside in constant address space}}
+int G5, G6;// expected-error 2 {{program scope variable must reside in constant address space}}
static float g_implicit_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
static constant float g_constant_static_var = 0;
static global float g_global_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
-static local float g_local_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static local float g_local_static_var = 0; // expected-error {{'__local' variable cannot have an initializer}}
static private float g_private_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
static generic float g_generic_static_var = 0; // expected-error{{OpenCL C version 1.2 does not support the 'generic' type qualifier}} // expected-error {{program scope variable must reside in constant address space}}
Index: test/SemaOpenCL/storageclass-cl20.cl
===
--- test/SemaOpenCL/storageclass-cl20.cl
+++ test/SemaOpenCL/storageclass-cl20.cl
@@ -2,12 +2,12 @@
int G2 = 0;
global int G3 = 0;
-local int G4 = 0; // expected-error{{program scope variable must reside in global or constant address space}}
+local int G4 = 0; // expected-error{{'__local' variable cannot have an initializer}}
static float g_implicit_static_var = 0;
static constant float g_constant_static_var = 0;
static
