[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:
> > 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.
> > 
> > 
> > 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.
> 
> Yes, this is how it's defined. Converting explicitly between two ASes where either one is a superset of the other is always legal.
> 
> > Just to understand in the current patch when are we supposed to use isAddressSpaceOverlapping and when isExplicitAddrSpaceConversionLegal. Can't we just always use isExplicitAddrSpaceConversionLegal?
> 
> I guess the distinction between `isAddressSpaceOverlapping` and `isExplicitAddrSpaceConversionLegal` is pretty subtle. You would want the former when you need to know if **implicit conversion A->B or B->A** is permitted, and the latter when you need to know if **explicit conversion A->B** is permitted.
> 
> Though in most cases, I think the latter is probably the most common.
> 
> > 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?
> 
> No, the current behavior of permitting all explicit conversions is according to spec: "A non-null pointer into an address space A can be cast to a pointer into another address space B, but such a cast is undefined if the source pointer does not point to a location in B." The addition of `isExplicitAddrSpaceConversionLegal` lets targets override this behavior and make such casts non-legal.
I see so we are making the new rules then. I guess it should be fine. However we might want to document this better and explain the difference between two types of API provided.



================
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:
> > 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?
> > Ok, but shouldn't we test the new functionality of rejecting C style casts if target doesn't permit the conversion?
> 
> Absolutely, but there are no targets which use the functionality yet, and I wouldn't want to make any such change without consulting with a target owner first.
> 
> > Also I believe C style cast functionality is being handled in TryAddressSpaceCast, and it probably belongs there.  Why is this change here needed? What test case does it cover?
> 
> That's a good point! Address space conversion for c-style casts should have already been handled by the time we get to TryReinterpretCast. So doesn't that mean that this code path is dead in trunk as well?
> 
> Absolutely, but there are no targets which use the functionality yet, and I wouldn't want to make any such change without consulting with a target owner first.
> 
Alternatively we can add a flag just for testing to override the address space settings for targets. Or may be we could just reuse `-ffake-address-space-map`.  I believe I've mentioned it before.


> That's a good point! Address space conversion for c-style casts should have already been handled by the time we get to TryReinterpretCast. So doesn't that mean that this code path is dead in trunk as well?

Yeah, possibly we have overlooked that.


================
Comment at: lib/Sema/SemaOverload.cpp:3171
+  // qualification conversion for disjoint address spaces make address space
+  // casts *work*?
   Qualifiers FromQuals = FromType.getQualifiers();
----------------
ebevhan wrote:
> Anastasia wrote:
> > 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?
> I still don't really understand why the code checks for overlap here. Removing this check causes one test case to break, CodeGenCXX/address-space-cast.cpp. Specifically, this:
> 
> ```
> #define __private__ __attribute__((address_space(5)))
> 
> void test_cast(char *gen_char_ptr, void *gen_void_ptr, int *gen_int_ptr) {
>   __private__ void *priv_void_ptr = (__private__ void *)gen_char_ptr;
> }
> ```
> 
> It tries to resolve the C-style cast with TryAddressSpaceCast, but fails as the underlying pointee types (`char` and `void`) are different. Then it tries to do it as a static_cast instead, but fails to produce an `AddressSpaceConversion`; instead, it makes a `BitCast` **as the second conversion step** which causes CodeGen to break since the conversion kind is wrong (the address spaces don't match).
> 
> Is address space conversion supposed to be a pointer conversion or a qualification conversion? If the second conversion step had emitted an AddressSpaceConversion instead of a BitCast, it would have worked. But at the same time, if we have IsQualificationConversion return false whenever we would need to do an address space conversion, other tests break.
> 
> I suppose that a solution might be to remove the special case in IsQualificationConversion for address spaces, and that TryAddressSpaceCast should ignore the underlying type if it's part of a C-style cast. That way we won't try to handle it as a static_cast. But I don't know if that's just a workaround for a larger underlying issue, or if it causes other issues.
> 
> All in all, I have no idea what these code paths are trying to accomplish. It just doesn't make sense to me. :(
> 
> It tries to resolve the C-style cast with TryAddressSpaceCast, but fails as the underlying pointee types (char and void) are different. Then it tries to do it as a static_cast instead, but fails to produce an AddressSpaceConversion; instead, it makes a BitCast as the second conversion step which causes CodeGen to break since the conversion kind is wrong (the address spaces don't match).

Strange! `TryStaticCast` should set cast kind to `CK_AddressSpaceConversion` if it detects the address spaces are different. Do you know why it doesn't happen for this test case?

> Is address space conversion supposed to be a pointer conversion or a qualification conversion?

It is a qualification conversion.

> 
> I suppose that a solution might be to remove the special case in IsQualificationConversion for address spaces, and that TryAddressSpaceCast should ignore the underlying type if it's part of a C-style cast. That way we won't try to handle it as a static_cast. But I don't know if that's just a workaround for a larger underlying issue, or if it causes other issues.

I think the idea of separate conversions is for each of them to work as a separate step. So address space cast should be working just as const cast i.e. it should only check the address space conversion and not the type. However the problem here is that `addrspacecast` supersedes `bitcast`. Therefore there are extra checks in the casts later to classify the cast kind correctly. If this flow fails we can reevaluate but this is the idea we were following up to now.










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

https://reviews.llvm.org/D62574