Page MenuHomePhabricator

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

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

Details

Reviewers
rjmccall
yaxunl
Summary

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
lib/Sema/SemaCast.cpp
2286

Why is this not a method on CastOperation?

2291

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
lib/Sema/SemaCast.cpp
2286

Will change!

2291

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
lib/Sema/SemaCast.cpp
2291

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

rjmccall added inline comments.Oct 5 2018, 9:56 AM
lib/Sema/SemaCast.cpp
2291

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
lib/Sema/SemaCast.cpp
2291

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

LGTM.

lib/Sema/SemaCast.cpp
2291

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
lib/Sema/SemaCast.cpp
2291

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.