[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+                             /*BasePath=*/nullptr, CCK)
+               .get();
 
----------------
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Anastasia wrote:
> > > > > rjmccall wrote:
> > > > > > Okay.  But if `ToType` *isn't* a reference type, this will never be an address-space conversion.  I feel like this code could be written more clearly to express what it's trying to do.
> > > > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > > > The logic is wrong for pointer types; if you're converting pointers, you need to be checking the address space of the pointee type of the from type.
> > > > 
> > > > It sounds like this is totally inadequately tested; please flesh out the test with all of these cases.  While you're at it, please ensure that there are tests verifying that we don't allowing address-space changes in nested positions.
> > > Thanks for spotting this bug! The generated IR for the test was still correct because AS of `FromType` happened to correctly mismatch AS of pointee of `ToType`.
> > > 
> > > I failed to construct the test case where it would miss classifying `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case in which `addrspacecast` was incorrectly added for pointers where it wasn't needed (see line 36 of the test). I think this code is covered now.
> > > 
> > > As for the address space position in pointers, the following test checks the address spaces of pointers in `addrspacecast`. For the other program paths we also have a test with similar checks in `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++ mode too.
> > > 
> > > BTW, while trying to construct a test case for the bug, I have discovered that multiple pointer indirection casting isn't working correctly. I.e. for the following program:
> > >   kernel void foo(){
> > >      __private int** loc;
> > >      int** loc_p = loc;
> > >      **loc_p = 1;
> > >   }
> > > We generate:
> > >   bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> > > in OpenCL C and then perform `store` over pointer in AS 4 (generic). We have now lost the information that the original pointer was in `private` AS and that the adjustment of AS segment has to be performed before accessing memory pointed by the pointer. Based on the current specification of `addrspacecast` in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not very clear whether it can be used for this case without any modifications or clarifications and also what would happen if there are multiple AS mismatches. I am going to look at this issue separately in more details. In OpenCL C++ an ICE is triggered for this though. Let me know if you have any thoughts on this.
> > Thanks, the check looks good now.
> > 
> > > BTW, while trying to construct a test case for the bug, I have discovered that multiple pointer indirection casting isn't working correctly.
> > 
> > This needs to be an error in Sema.  The only qualification conversions that should be allowed in general on nested pointers (i.e. on `T` in `T**` or `T*&`) are the basic C qualifiers: `const`, `volatile`, and `restrict`; any other qualification change there is unsound.
> I see. I guess it's because C++ rules don't cover address spaces.
> 
> It feels like it would be a regression for OpenCL C++ vs OpenCL C to reject nested pointers with address spaces because it was allowed before. :(
> 
> However, the generation for OpenCL C and C are incorrect currently. I will try to sort that all out as a separate patch though, if it makes sense? 
C++'s rules assume that qualifiers don't introduce real representation differences and that operations on qualified types are compatible with operations on unqualified types.  That's not true of qualifiers in general: address space qualifiers can change representations, ARC qualifiers can have incompatible semantics, etc.  There is no way to soundly implement a conversion from `__private int **` to `__generic int **`, just there's no way to soundly implement a conversion from `Derived **` to `Base **`.

If you want to allow this conversion anyway for source-compatibility reasons (and I don't think that's a good idea), it should be a bitcast.


https://reviews.llvm.org/D53764