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

[Bevin Hansson via Phabricator via cfe-commits]

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

Sorry for the very late response on this. Hope it's not completely off your radar.



================
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:
> 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.
> 
I will add some more comments to the functions to describe a bit better what they should be used for.


================
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:
> 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.
I haven't looked more into the testing issue yet.

----

I think that reinterpret_casts for c-style casts must handle AS conversions like this as it must be possible to do a conversion like `AS1 T1 * -> AS2 T2 *` where:
* T1 is not a derived of T2
* T2 is not void

and AS2 is not a superspace of AS1.

In such cases neither addrspace_cast nor static_cast apply, so reinterpret_cast must be able to handle explicit AS conversions.


================
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:
> > > 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.
> 
> 
> 
> 
> 
> 
> 
> 
I've determined that something doesn't add up with how conversions are generated. I'm pretty sure that the issue lies in `PerformImplicitConversion`.

When we analyze the c-style cast in this:
```
char * p;
__private__ void * pp = (__private__ void *)gen_char_ptr;
```
we determine that this cannot be an addrspace_cast (as the unqualified pointee types are different). It can be a static_cast, though. The implicit conversion resulting from this static_cast should occur in two steps: the second conversion should be a pointer conversion from `char*` to `void*`, and the third conversion should be a qualification conversion from `void*` to `__private__ void*`.

Now, this *is* the ICS/SCS that we get. However, when we realize the conversion sequence in PerformImplicitConversion, it actually completely ignores the intermediate types in the conversion sequence. Everything it does is only on the intermediate 'From' type after each conversion step and the final 'To' type, but we never consider the intermediate 'To' type. This means that it will try to realize the entire conversion sequence (both the 'bitcast' conversion and the 'addrspace' conversion) in a single (pointer conversion) step instead of two, and since CheckPointerConversion has no provisions for address space casts (because why should it?) we get a CK_BitCast and codegen fails since it's trying to change the address space of the type.

This means that each conversion step realization needs to be aware of every kind of CastKind that could possibly occur in each step, which is bizarre. We know exactly what type we are converting from and to in each conversion in the sequence; why do we use the *final* result type to determine what kind of implicit cast we should use in each step?

I've tried to make PerformImplicitConversion aware of the intermediate types, but this breaks a bunch of tests, as there are lots of checks and diagnoses in the function that require the final type. One contributor to this is that the 'ToType' of the SCS (`ToType[2]`) doesn't actually have to match the final 'ToType' of the conversion. Something to do with exception specifications.

It feels like this code is just broken and everyone has just been coding around the issues. Perhaps this is the root:
```
  // Overall FIXME: we are recomputing too many types here and doing far too
  // much extra work. What this means is that we need to keep track of more
  // information that is computed when we try the implicit conversion initially,
  // so that we don't need to recompute anything here.
```

I don't know what the solution here is, short of redesigning PerformImplicitConversion.


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

https://reviews.llvm.org/D62574