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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
