[PATCH] D62574: Initial draft of target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 3 inline comments as done.
ebevhan added a comment.

In D62574#1552220 <https://reviews.llvm.org/D62574#1552220>, @Anastasia wrote:

> Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.


That functionality is actually enabled by default to allow for Clang's current semantics. Embedded-C allows all explicit conversions by default, and OpenCL in Clang allows all explicit conversion to and from target spaces as an extension.

It's rather up to each target to *disable* the default explicit conversion behavior if they don't want it.



================
Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+    // A overlaps with B if either is a superset of the other.
----------------
Anastasia wrote:
> Is there any advantage of keeping superset&subset concept? Amd if yes, how do we position the new functionality with explicit cast?
> 
> I think I am missing a bit conceptual view... because I think we originally discussed to switch to implicit/explicit conversion model. Perhaps there is no reason to do it but I would just like to understand why? 
Yes, I know the original discussion was regarding the implicit/explicit model, but I came to realize during the implementation that all that was needed to get the superspace model to work generically with the current behavior was an override on the explicit conversion.

The implicit/explicit model also has the drawback that it's a bit too expressive. You can express semantics that just don't really make sense, like permitting implicit conversion but not explicit conversion. The superspace model doesn't let you do that, and the additions I've made here still don't: If implicit conversion is allowed, explicit conversion must also be allowed. It just becomes possible to allow explicit conversion for ASes that don't overlap.

Since the superspace model is what OpenCL and Embedded-C use in their specification, it's probably better to use that anyway.


================
Comment at: lib/Sema/SemaCast.cpp:2224
+    // the cast is explicitly legal as well.
+    if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+               : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {
----------------
Anastasia wrote:
> It seems like you are changing the functionality here. Don't we need any test for this?
I don't think it's necessarily changing. If we are doing a reinterpret_cast that stems from a c-style cast, we want to check that explicit conversion is allowed. This happens both if either AS overlaps, or if the target permits it. If it's not a C-style cast, then we need to check for subspace correctness instead, as reinterpret_cast can only do 'safe' casts.

The original code here allows all explicit C-style casts regardless of AS, but now we have to ask the target first.


================
Comment at: lib/Sema/SemaCast.cpp:2319
+
+  Kind = CK_AddressSpaceConversion;
+  return TC_Success;
----------------
Anastasia wrote:
> Btw I think this is set in:
> https://reviews.llvm.org/D62299
> 
> Although I don't have any objections to changing to this.
Oh, indeed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574