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