ebevhan added a comment. In D62574#2183294 <https://reviews.llvm.org/D62574#2183294>, @Anastasia wrote:
> 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? My cache is a bit flushed on this at the moment but I do believe that if the issue described in D83325 <https://reviews.llvm.org/D83325> is resolved, this patch should work. ================ 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: > > > 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 That's neat. It does seem like the example in D83325 compiles too, but I suspect it wouldn't work with this patch. 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
