Anastasia added a comment. In D62574#1534159 <https://reviews.llvm.org/D62574#1534159>, @ebevhan wrote:
> >> 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? > > Well, since there is a mapping from the OpenCL address spaces to the AMDGPU > target spaces, I would presume that if the target spaces were used on their > own (via `__attribute__((address_space(1)))` for example) they should behave > similarly to their OpenCL counterparts. It wouldn't have to be much, just a > couple of type definitions and checks for implicit/explicit cast behavior. > > There's also the x86 case I mentioned. I'm not really sure how that feature > is used, though. Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces. ================ 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. ---------------- 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? ================ 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)) { ---------------- It seems like you are changing the functionality here. Don't we need any test for this? ================ Comment at: lib/Sema/SemaCast.cpp:2319 + + Kind = CK_AddressSpaceConversion; + return TC_Success; ---------------- Btw I think this is set in: https://reviews.llvm.org/D62299 Although I don't have any objections to changing to this. ================ Comment at: lib/Sema/SemaCast.cpp:2334 // warn even though local -> generic is permitted. - if (Self.getLangOpts().OpenCL) { - const Type *DestPtr, *SrcPtr; ---------------- I think you might need to update the comment above to reflect that you are generalizing the behavior. 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
