Page MenuHomePhabricator

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

Authored by Anastasia on Sep 27 2018, 4:00 AM.



C style cast in OpenCL C++ was ignoring the address space conversions from OpenCL C and as a result accepting incorrect code to compile. This commit adds correctness checking for address spaces. This makes all OpenCL C address space conversion tests from Sema passing in -cl-std=c++ mode too. Error messages have to be changed to C++ style in multiple places, but semantics of the diagnostics stay the same.

Note: this doesn't fix C++ cast operators. OpenCL C++ s3.4.4 defines special address space cast operators that align with C++'s philosophy of safe casts. The motivation behind this is that address space casts might require address translation (denoted by the addrspacecast IR instruction). In order to avoid operations modifying the type or constness together with translation of addresses, a separate cast operator is introduced. We might still have to fix the semantics of address spaces in static_cast, reinterpret_cast and const_cast but it's currently unclear to me. I will address this as a separate patch.

Diff Detail

Event Timeline

Anastasia created this revision.Sep 27 2018, 4:00 AM
rjmccall added inline comments.Oct 1 2018, 11:34 AM

Why is this not a method on CastOperation?


Please test the result of getAs instead of separately testing isPointerType.

Why is this check OpenCL-specific? Address spaces are a general language feature.

Anastasia added inline comments.Oct 2 2018, 10:26 AM

Will change!


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!

Anastasia updated this revision to Diff 168471.Oct 5 2018, 7:37 AM

Change AS checking function to be a method of CastOperation.

Anastasia added inline comments.Oct 5 2018, 7:53 AM

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:


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

rjmccall added inline comments.Oct 5 2018, 9:56 AM

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.

Anastasia updated this revision to Diff 168633.Oct 8 2018, 3:32 AM
  • Changed to pass QualType by value.
  • Improved description of new method.
  • Added a comment about the OpenCL rules.
Anastasia added inline comments.Oct 8 2018, 4:02 AM

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.

rjmccall accepted this revision.Oct 8 2018, 10:49 AM



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.

This revision is now accepted and ready to land.Oct 8 2018, 10:49 AM
Anastasia added inline comments.Oct 9 2018, 3:03 AM

Ok, I see. Even if they seem to cover variety of different memory architectures, it probably doesn't make sense to have arbitrary conversions indeed. I think heterogeneous C++ group was looking at regulating the address spaces. I will try to get in touch with them and investigate.

Anastasia closed this revision.Oct 10 2018, 9:09 AM

Committed in r344148. The review link was forgotten and therefore didn't get closed automatically.