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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
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.
----------------
ebevhan wrote:
> 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.
> 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.

Ok, I think we could define the new model something like - explicit conversions are automatically allowed for all implicit conversions... targets won't have to specify those but only extra comversions that are not allowed implicitly. 

Just to understand in the current patch when are we supposed to use `isAddressSpaceOverlapping` and when `isExplicitAddrSpaceConversionLegal`. Can't we just always use `isExplicitAddrSpaceConversionLegal`?

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

I agree the advantage of following spec is really huge. However, Clang is already broken for Emdedded C isn't it? Because it allows any explicit conversions?

As for OpenCL it might be reasonable to provide new documentation if needed as soon as the new rules don't invalidate all behavior.




================
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)) {
----------------
ebevhan wrote:
> 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.
Ok, but shouldn't we test the new functionality of rejecting C style casts if target doesn't permit the conversion?

Also I believe C style cast functionality is being handled in TryAddressSpaceCast, and it probably belongs there. In fact you have already extended this:

```
if (!DestPtrType->isAddressSpaceOverlapping(*SrcPtrType)) {
    msg = diag::err_bad_cxx_cast_addr_space_mismatch;
    return TC_Failed;
}
```

Why is this change here needed? What test case does it cover?


================
Comment at: lib/Sema/SemaCast.cpp:2325
+  // Only conversions between pointers to objects in overlapping
+  // addr spaces are allowed, unless the target explicitly allows it.
 
----------------
I would keep the OpenCL part but move it to the end as an example. It often helps when there are spec references in the code. 


```
// OpenCL v2.0 s6.5.5 - Generic addr space overlaps with any named one, except for constant.

```


================
Comment at: lib/Sema/SemaExpr.cpp:10616
         const PointerType *LHSPtr = LHSType->getAs<PointerType>();
-        if (!LHSPtr->isAddressSpaceOverlapping(*RHSType->getAs<PointerType>())) {
+        if (!Context.isAddressSpaceOverlapping(
+              LHSPtr->getPointeeType().getAddressSpace(),
----------------
Should this also be allowed if pointers are explicitly convertible?


================
Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();
----------------
Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > I guess if address spaces don't overlap we don't have a valid qualification conversion. This seems to align with the logic for cv. My guess is that none of the other conversions will be valid for non overlapping address spaces and the error will occur.
> > > 
> > > I think at this point we might not need to know if it's implicit or explicit? I believe we might have a separate check for this somewhere because it works for OpenCL. I don't know though if it might simplify the flow if we move this logic rather here.
> > > 
> > > The cv checks above seem to use `CStyle` flag. I am wondering if we could use it to detect implicit or explicit. Because we can only cast address space with C style cast at the moment.  Although after adding `addrspace_cast` operator that will no longer be the only way.
> > > I guess if address spaces don't overlap we don't have a valid qualification conversion. This seems to align with the logic for cv. My guess is that none of the other conversions will be valid for non overlapping address spaces and the error will occur.
> > 
> > Right. So the reasoning is that if the address spaces are disjoint according to the overlap rule, then it cannot be considered a qualification conversion.
> > 
> > But with the new hooks, it's possible for a target to allow explicit conversion even if address spaces do not overlap according to the rules. So even though there is no overlap, such a conversion could still be a qualification conversion if it was explicit (either via a c-style cast or an `addrspace_cast`). This is in fact the default for all targets (see the patch in TargetInfo.h).
> > 
> > I think I need a refresher on how the casts were meant to work; were both `static_cast` and `reinterpret_cast` supposed to be capable of implicit conversion (say, private -> generic) but only `addrspace_cast` for the other direction (generic -> private)? Or was `reinterpret_cast` supposed to fail in the first case?
> Just as a summary:
> 
> - All casts should allow safe/implicit AS conversions i.e. `__private`/`__local`/`__global` -> `__generic` in OpenCL
> - All casts except for C-style and `addrspace_cast` should disallow unsafe/explicit conversions i.e. generic -> `__private`/`__local`/`__global` in OpenCL
> - All casts should disallow forbidden conversions with no address space overlap i.e. `__constant` <-> any other in OpenCL
> 
> In OpenCL overlapping logic is only used for explicit i.e. unsafe conversion. So it seems we might only need those conversions here then? 
Did you have time to look into this?


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

https://reviews.llvm.org/D62574