[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:
> > > > 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.


https://reviews.llvm.org/D53764