Anastasia marked 4 inline comments as done.
Anastasia added a comment.
Do you think there is anything else to do for this patch?
================
Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+ .get();
----------------
rjmccall wrote:
> 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.
Ok, then `bitcast` is not a good solution because it has an issue of loosing
address space information. Perhaps disallowing it completely is a better
approach in this case. I have created a bug to investigate it further and may
be request some feedback from other OpenCL developers:
https://bugs.llvm.org/show_bug.cgi?id=39674
https://reviews.llvm.org/D53764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
