Anastasia added a comment. In D62574#1527126 <https://reviews.llvm.org/D62574#1527126>, @ebevhan wrote:
> In D62574#1523835 <https://reviews.llvm.org/D62574#1523835>, @Anastasia wrote: > > > > This patch does not address the issue with the accessors > > > on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes), > > > because I don't know how to solve it without breaking a ton of > > > rather nice encapsulation. Either, every mention of compatiblyIncludes > > > must be replaced with a call to a corresponding ASTContext method, > > > Qualifiers must have a handle to ASTContext, or ASTContext must be > > > passed to every such call. This revision mentions the issue in a comment: > > > https://reviews.llvm.org/D49294 > > > > I think using ASTContext helper is ok then. > > > Alright. Just to clarify, you mean the first option (using a new accessor on > ASTContext and replacing all existing compatiblyIncludes with that)? Yes, it seems ok to me. Not sure if @rjmccall has an opinion. > I'll have to see if that's possible without breaking a few more interfaces, > since you can throw around Qualifiers and check for compatibility without an > ASTContext today. > >> I was just thinking about testing the new logic. Should we add something >> like `-ffake-address-space-map` >> (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that >> will force targets to use some implementation of fake address space >> conversions? It was quite useful in OpenCL before we added concrete targets >> with address spaces. Alternatively, we can try to use SPIR that is a generic >> Clang-only target that has address spaces. > > Well, there are a couple targets which have target address spaces even today, > I think. AMDGPU should be one, right? Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test? > I forgot to mention; this patch also disables two "extensions" that Clang > has, namely that subtraction and comparison on pointers of different address > spaces is legal regardless of address space compatibility. I'm pretty sure > that these extensions only exist because Clang hasn't had support for targets > to express address space compatibility until now. According to the docs, x86 > uses address spaces for certain types of segment access: > https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments > > If x86 (or any target that uses target address spaces) wants to keep using > this and still keep such pointers implicitly compatible with each other, it > will need to configure this in its target hooks. Ok, it seems logical to me. In OpenCL we don't allow those cases. ================ 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: > > 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? ================ Comment at: lib/Sema/SemaOverload.cpp:5150 + // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is + // not qualified with an address space, but if there's no implicit conversion ---------------- ebevhan wrote: > Anastasia wrote: > > Agree! We need to check that address spaces are not identical and then > > validity! > > > > But I guess it's ok for C++ because we can't add addr space qualifier to > > implicit object parameter yet? > > > > It's broken for OpenCL though! > > But I guess it's ok for C++ because we can't add addr space qualifier to > > implicit object parameter yet? > > True, but that will become possible eventually. > > I'm not sure if this is currently an issue with OpenCL as it is either. It's > only a problem if FromType is not qualified, but is that possible? Even the > 'notational' non-AS-qualification in OpenCL is still a qualification under > the hood (either private or generic). Or maybe I'm misunderstanding how it > works. Yes, ideally we deduce address spaces if they are not specified explicitly. However I am not sure this is currently the case everywhere and whether we can have weird interplay with templates logic. One case I have in mind is function return type that is kept with `Default` address space but it shouldn't ever occur with the logic here. So perhaps theoretically it's not broken for OpenCL. :) Repository: rC Clang 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
