[OpenCL] Fixed parsing of address spaces for C++

[Anastasia]

Summary

Due to missing handling of address space tokens in parsing code of C++ we were unable to parse declarations that start from an address space keyword. For example we would get an error:

test.cl:2:1: error: expected expression
__global int  * arg_glob;

No idea if there are some more cases missing but this patch at least fixes basic variable and function argument declaration parsing.

I enable address space test but part of it still can't run correctly in C++ mode.

[yaxunl]

Did you notice the bug that C++ silently allows implicit casting of a pointer to a pointer type with different address space?

Do you have a fix for that?

[Anastasia]

In D48419#1139601, @yaxunl wrote:

Did you notice the bug that C++ silently allows implicit casting of a pointer to a pointer type with different address space?

Do you have a fix for that?

I didn't think it did because if I run test/SemaOpenCL/address-spaces-conversions-cl2.0.cl in -cl-std=c++ it gives me errors when I initialize a pointer with different AS pointer:

error: cannot initialize a variable of type '__global int *' with an lvalue of type '__local int *'
error: assigning to '__global int *' from incompatible type '__local int *'
error: comparison of distinct pointer types ('__global int *' and '__local int *')

Do you have an example code somewhere?

[yaxunl]

In D48419#1139749, @Anastasia wrote:

In D48419#1139601, @yaxunl wrote:

Did you notice the bug that C++ silently allows implicit casting of a pointer to a pointer type with different address space?

Do you have a fix for that?

I didn't think it did because if I run test/SemaOpenCL/address-spaces-conversions-cl2.0.cl in -cl-std=c++ it gives me errors when I initialize a pointer with different AS pointer:

error: cannot initialize a variable of type '__global int *' with an lvalue of type '__local int *'
error: assigning to '__global int *' from incompatible type '__local int *'
error: comparison of distinct pointer types ('__global int *' and '__local int *')

Do you have an example code somewhere?

Actually this only happens when there is an explicit cast:

$ cat address-space-cast.cpp
// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck %s

#define __private__ __attribute__((address_space(5)))

void f(char* q) {
  // CHECK: addrspacecast
  __private__ char* p = (__private__ char*)q;
  //__private__ char* p = q;
}
$clang -cc1 -triple=amdgcn-amd-amdhsa address-space-cast.cpp -ast-dump
TranslationUnitDecl 0x699f418 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x699fab0 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x699f770 '__int128'
|-TypedefDecl 0x699fb18 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x699f790 'unsigned __int128'
|-TypedefDecl 0x699fe58 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x699fc00 '__NSConstantString_tag'
|   `-CXXRecord 0x699fb68 '__NSConstantString_tag'
|-TypedefDecl 0x699fef0 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'char *'
| `-PointerType 0x699feb0 'char *'
|   `-BuiltinType 0x699f4b0 'char'
`-FunctionDecl 0x69a0020 <address-space-cast.cpp:5:1, line:9:1> line:5:6 f 'void (char *)'
  |-ParmVarDecl 0x699ff58 <col:8, col:14> col:14 used q 'char *'
  `-CompoundStmt 0x69e4130 <col:17, line:9:1>
    `-DeclStmt 0x69e4118 <line:3:21, line:7:45>
      `-VarDecl 0x69e4000 <line:3:21, line:7:44> col:21 p '__attribute__((address_space(5))) char *' cinit
        `-CStyleCastExpr 0x69e40f0 <col:25, col:44> '__attribute__((address_space(5))) char *' <NoOp>
          `-ImplicitCastExpr 0x69e40d8 <col:44> '__attribute__((address_space(5))) char *' <NoOp>
            `-ImplicitCastExpr 0x69e40c0 <col:44> 'char *' <LValueToRValue>
              `-DeclRefExpr 0x69e4080 <col:44> 'char *' lvalue ParmVar 0x699ff58 'q' 'char *'

In the AST there is a CStyleCast and an ImplicitCast, both are NoOp, which will becomes bitcast in codegen and causes invalid bitcast instruction.

[Anastasia]

In D48419#1139799, @yaxunl wrote:

In D48419#1139749, @Anastasia wrote:

In D48419#1139601, @yaxunl wrote:

Did you notice the bug that C++ silently allows implicit casting of a pointer to a pointer type with different address space?

Do you have a fix for that?

I didn't think it did because if I run test/SemaOpenCL/address-spaces-conversions-cl2.0.cl in -cl-std=c++ it gives me errors when I initialize a pointer with different AS pointer:

error: cannot initialize a variable of type '__global int *' with an lvalue of type '__local int *'
error: assigning to '__global int *' from incompatible type '__local int *'
error: comparison of distinct pointer types ('__global int *' and '__local int *')

Do you have an example code somewhere?

Actually this only happens when there is an explicit cast:

$ cat address-space-cast.cpp
// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck %s

#define __private__ __attribute__((address_space(5)))

void f(char* q) {
  // CHECK: addrspacecast
  __private__ char* p = (__private__ char*)q;
  //__private__ char* p = q;
}
$clang -cc1 -triple=amdgcn-amd-amdhsa address-space-cast.cpp -ast-dump
TranslationUnitDecl 0x699f418 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x699fab0 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x699f770 '__int128'
|-TypedefDecl 0x699fb18 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x699f790 'unsigned __int128'
|-TypedefDecl 0x699fe58 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x699fc00 '__NSConstantString_tag'
|   `-CXXRecord 0x699fb68 '__NSConstantString_tag'
|-TypedefDecl 0x699fef0 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'char *'
| `-PointerType 0x699feb0 'char *'
|   `-BuiltinType 0x699f4b0 'char'
`-FunctionDecl 0x69a0020 <address-space-cast.cpp:5:1, line:9:1> line:5:6 f 'void (char *)'
  |-ParmVarDecl 0x699ff58 <col:8, col:14> col:14 used q 'char *'
  `-CompoundStmt 0x69e4130 <col:17, line:9:1>
    `-DeclStmt 0x69e4118 <line:3:21, line:7:45>
      `-VarDecl 0x69e4000 <line:3:21, line:7:44> col:21 p '__attribute__((address_space(5))) char *' cinit
        `-CStyleCastExpr 0x69e40f0 <col:25, col:44> '__attribute__((address_space(5))) char *' <NoOp>
          `-ImplicitCastExpr 0x69e40d8 <col:44> '__attribute__((address_space(5))) char *' <NoOp>
            `-ImplicitCastExpr 0x69e40c0 <col:44> 'char *' <LValueToRValue>
              `-DeclRefExpr 0x69e4080 <col:44> 'char *' lvalue ParmVar 0x699ff58 'q' 'char *'

In the AST there is a CStyleCast and an ImplicitCast, both are NoOp, which will becomes bitcast in codegen and causes invalid bitcast instruction.

Yes this extra ImplicitCastExpr doesn't appear in OpenCL C and also CStyleCastExpr is correctly attributed as AddressSpaceConversion

CStyleCastExpr 0xc490810 <col:23, col:40> 'char *' <AddressSpaceConversion>

Since C++ uses conversion ranking rules the Sema code for it largely goes through the different path than in C. I think fixing address spaces in C++ or OpenCL C++ will need quite a bit of more work. I am planning to look at further issues in the next weeks, but not sure when...

I hope it doesn't prevent us to go ahead with this parsing fix.

[yaxunl]

LGTM. Thank!

lib/Parse/ParseTentative.cpp

     // cv-qualifier
   case tok::kw_const:
   case tok::kw_volatile:
+  case tok::kw___private:
+  case tok::kw___local:
+  case tok::kw___global:
+  case tok::kw___constant:
+  case tok::kw___generic:
 
     // GNU
   case tok::kw_restrict:

test/SemaOpenCL/address-spaces.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -cl-std=c++ -verify -pedantic -fsyntax-only
 
 __constant int ci = 1;
 

   __local int lj = 2; // expected-error {{'__local' variable cannot have an initializer}}
 
   int *ip;
+// FIXME: Temporarily disable part of the test that doesn't work for C++ yet.
+#if !__OPENCL_CPP_VERSION__
 #if __OPENCL_C_VERSION__ < 200
   ip = gip; // expected-error {{assigning '__global int *' to 'int *' changes address space of pointer}}
   ip = &li; // expected-error {{assigning '__local int *' to 'int *' changes address space of pointer}}

   __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
+#endif // !__OPENCL_CXX_VERSION__