[OpenCL] Postpone PSV address space diagnostic

[svenvh]

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.

[rjmccall]

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.

[svenvh]

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.

[rjmccall]

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

[Anastasia]

In D53705#1279086, @rjmccall wrote:

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

[rjmccall]

In D53705#1280264, @Anastasia wrote:

In D53705#1279086, @rjmccall wrote:

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

[keryell]

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

[keryell]

In 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. :-)

[rjmccall]

In D53705#1281738, @keryell wrote:

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

[Anastasia]

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.

[keryell]

In 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. :-)

[rjmccall]

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

[keryell]

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

[rjmccall]

In D53705#1284734, @keryell wrote:

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

[keryell]

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

[rjmccall]

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.

[Anastasia]

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?

lib/Sema/SemaDecl.cpp

if (T->isBlockPointerType()) {
         return;
       }
       if (NewVD->hasExternalStorage()) {
         Diag(NewVD->getLocation(), diag::err_opencl_extern_block_declaration);
         NewVD->setInvalidDecl();
         return;
       }
     }
-    // OpenCL v1.2 s6.5 - All program scope variables must be declared in the
-    // __constant address space.
-    // OpenCL v2.0 s6.5.1 - Variables defined at program scope and static
-    // variables inside a function can also be declared in the global
-    // address space.
-    if (NewVD->isFileVarDecl() || NewVD->isStaticLocal() ||
-        NewVD->hasExternalStorage()) {
-      if (!T->isSamplerT() &&
-          !(T.getAddressSpace() == LangAS::opencl_constant ||
-            (T.getAddressSpace() == LangAS::opencl_global &&
-             getLangOpts().OpenCLVersion == 200))) {
-        int Scope = NewVD->isStaticLocal() | NewVD->hasExternalStorage() << 1;
-        if (getLangOpts().OpenCLVersion == 200)
-          Diag(NewVD->getLocation(), diag::err_opencl_global_invalid_addr_space)
-              << Scope << "global or constant";
-        else
-          Diag(NewVD->getLocation(), diag::err_opencl_global_invalid_addr_space)
-              << Scope << "constant";
-        NewVD->setInvalidDecl();
-        return;
-      }
-    } else {
+    if (!NewVD->isFileVarDecl() && !NewVD->isStaticLocal() &&
+        !NewVD->hasExternalStorage()) {
       if (T.getAddressSpace() == LangAS::opencl_global) {
         Diag(NewVD->getLocation(), diag::err_opencl_function_variable)
             << 1 /*is any function*/ << "global";
         NewVD->setInvalidDecl();
         return;
       }
       if (T.getAddressSpace() == LangAS::opencl_constant ||
           T.getAddressSpace() == LangAS::opencl_local) {
if (Decl *D = Group[i]) {
                  diag::err_auto_non_deduced_not_alone)
                 << FirstNonDeducedAutoInGroup->getType()
                        ->hasAutoForTrailingReturnType()
                 << FirstDeclaratorInGroup->getSourceRange()
                 << DD->getSourceRange();
             DiagnosedNonDeducedAuto = true;
           }
         }
-      }
 
+        // OpenCL v1.2 s6.5 - All program scope variables must be declared in
+        // the __constant address space.
+        // OpenCL v2.0 s6.5.1 - Variables defined at program scope and static
+        // variables inside a function can also be declared in the global
+        // address space.
+        if (getLangOpts().OpenCL) {
+          if (VarDecl *NewVD = dyn_cast<VarDecl>(D)) {
+            if (!D->isInvalidDecl()) {
+              QualType T = NewVD->getType();
+              if (NewVD->isFileVarDecl() || NewVD->isStaticLocal() ||
+                  NewVD->hasExternalStorage()) {
+                if (!T->isSamplerT() &&
+                    !(T.getAddressSpace() == LangAS::opencl_constant ||
+                      (T.getAddressSpace() == LangAS::opencl_global &&
+                       getLangOpts().OpenCLVersion == 200))) {
+                  int Scope =
+                      NewVD->isStaticLocal() | NewVD->hasExternalStorage() << 1;
+                  if (getLangOpts().OpenCLVersion == 200)
+                    Diag(NewVD->getLocation(),
+                         diag::err_opencl_global_invalid_addr_space)
+                        << Scope << "global or constant";
+                  else
+                    Diag(NewVD->getLocation(),
+                         diag::err_opencl_global_invalid_addr_space)
+                        << Scope << "constant";
+                  NewVD->setInvalidDecl();
+                }
+              }
+            }
+          }
+        }
+      }
       Decls.push_back(D);
     }
   }
 
   if (DeclSpec::isDeclRep(DS.getTypeSpecType())) {
     if (TagDecl *Tag = dyn_cast_or_null<TagDecl>(DS.getRepAsDecl())) {
       handleTagNumbering(Tag, S);
       if (FirstDeclaratorInGroup && !Tag->hasNameForLinkage() &&

test/SemaOpenCL/storageclass-cl20.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
 
 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 global float g_global_static_var = 0;
-static local float g_local_static_var = 0;     // expected-error {{program scope variable must reside in global or 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 global or constant address space}}
 static generic float g_generic_static_var = 0; // expected-error {{program scope variable must reside in global or constant address space}}
 
 extern float g_implicit_extern_var;
 extern constant float g_constant_extern_var;
 extern global float g_global_extern_var;
 extern local float g_local_extern_var;     // expected-error {{extern variable must reside in global or constant address space}}
 extern private float g_private_extern_var; // expected-error {{extern variable must reside in global or constant address space}}
 extern generic float g_generic_extern_var; // expected-error {{extern variable must reside in global or constant address space}}
 
 void kernel foo() {
   constant int L1 = 0;
   local int L2;
   global int L3;                              // expected-error{{function scope variable cannot be declared in global address space}}
   generic int L4;                             // expected-error{{automatic variable qualified with an invalid address space}}
   __attribute__((address_space(100))) int L5; // expected-error{{automatic variable qualified with an invalid address space}}
 
   static float l_implicit_static_var = 0;
   static constant float l_constant_static_var = 0;
   static global float l_global_static_var = 0;
-  static local float l_local_static_var = 0;     // expected-error {{static local variable must reside in global or constant address space}}
+  static local float l_local_static_var = 0;     // expected-error {{'__local' variable cannot have an initializer}}
   static private float l_private_static_var = 0; // expected-error {{static local variable must reside in global or constant address space}}
   static generic float l_generic_static_var = 0; // expected-error {{static local variable must reside in global or constant address space}}
 
   extern float l_implicit_extern_var;
   extern constant float l_constant_extern_var;
   extern global float l_global_extern_var;
   extern local float l_local_extern_var;     // expected-error {{extern variable must reside in global or constant address space}}
   extern private float l_private_extern_var; // expected-error {{extern variable must reside in global or constant address space}}
   extern generic float l_generic_extern_var; // expected-error {{extern variable must reside in global or constant address space}}
 }

test/SemaOpenCL/storageclass.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
 
 static constant int G1 = 0;
 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}}
 
 extern float g_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
 extern constant float g_constant_extern_var;
 extern global float g_global_extern_var;   // expected-error {{extern variable must reside in constant address space}}
 extern local float g_local_extern_var;     // expected-error {{extern variable must reside in constant address space}}
 extern private float g_private_extern_var; // expected-error {{extern variable must reside in constant address space}}

test/SemaOpenCLCXX/invalid-qualifier.cl

+// 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.