Anastasia added inline comments.
================
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
----------------
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.
https://reviews.llvm.org/D53764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
