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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#2136423 <https://reviews.llvm.org/D62574#2136423>, @ebevhan wrote:

> It seems that D70605 <https://reviews.llvm.org/D70605> attempted to ameliorate the issues that I observed (pointer-conversion doing too much), but it didn't manage to solve the problem fully.

Can you explain what's left to be done?



================
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:
> > 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.
Btw the example that you provide here seems to compile in OpenCL now: https://godbolt.org/z/j9E3s4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62574