[PATCH] D48419: [OpenCL] Fixed parsing of address spaces for C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D48419#1139799, @yaxunl wrote:

> In https://reviews.llvm.org/D48419#1139749, @Anastasia wrote:
>
> > In https://reviews.llvm.org/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.


https://reviews.llvm.org/D48419