[PATCH] D52598: [OpenCL] Fixed address space cast in C style cast of C++ parsing

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: lib/Sema/SemaCast.cpp:2288
+      SrcType->isPointerType()) {
+    const PointerType *DestPtr = DestType->getAs<PointerType>();
+    if (!DestPtr->isAddressSpaceOverlapping(*SrcType->getAs<PointerType>())) {
----------------
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > Please test the result of `getAs` instead of separately testing `isPointerType`.
> > > > > 
> > > > > Why is this check OpenCL-specific?  Address spaces are a general language feature.
> > > > I think mainly because I am factoring out from the existing OpenCL check. But it probably makes sense that the semantics of this is not different in other languages. I will update it! Thanks!
> > > After playing with this for a bit longer I discovered that I have to keep the OpenCL check unfortunately.
> > > 
> > > I found this old commit (`d4c5f84`/`r129583`) that says:
> > >   C-style casts can add/remove/change address spaces through the reinterpret_cast mechanism.
> > > That's not the same as in OpenCL because it seems for C++ you can cast any AS to any other AS. Therefore, no checks are needed at all. I am not sure if we can come up with a common function for the moment.
> > > 
> > > The following  tests are checking this:
> > >    CodeGenCXX/address-space-cast.cpp
> > >    SemaCXX/address-space-conversion.cpp
> > > 
> > > Do you think it would make sense to rename this method with OpenCL-something or keep in case may be CUDA or some other languages might need similar functionality...
> > > 
> > I think you can leave it alone for now, but please add a comment explaining the reasoning as best you see it, and feel free to express uncertainty about the right rule.
> > 
> > Don't take `QualType` by `const &`, by the way.  It's a cheap-to-copy value type and should always be passed by value.
> My general understand of the address spaces in C and C++ that they are intended to be more general than in OpenCL (i.e. they don't reflect any particular memory system). Perhaps, it makes sense that we have more restrictions in OpenCL or other similar languages.
No, their primary use is to reflect underlying memory systems, like near vs. far pointers on 32-on-64 targets or specialized address spaces like `gs`-segment addressing on x86-64, and it doesn't make sense to allow arbitrary conversions any more than it does for OpenCL.  The Embedded C spec has rules about overlapping address spaces, and while it doesn't say that casts between non-overlapping address spaces are ill-formed, it probably ought to.  Regardless, if we've permitted arbitrary casts in the past, we'll need to investigate the source compatibility issues before imposing restrictions.

There have also been proposals for "superficial" address spaces that are eliminated during lowering which might have their own restrictions.


https://reviews.llvm.org/D52598