rjmccall added inline comments.
================ Comment at: lib/Sema/SemaOverload.cpp:9279 + (CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default)) + return true; + } ---------------- Anastasia wrote: > rjmccall wrote: > > rjmccall wrote: > > > This at least needs a comment explaining the rule you're trying to > > > implement. > > Okay, thanks for the clarification. I think this should just be part of > > `CompareImplicitConversionSequence`, right? It seems to me that you should > > prefer e.g. `int __private *` -> `int __private *` over `int __generic *` > > as a normal argument conversion as well. > > > > Also, can this be written in terms of `isAddressSpaceSupersetOf`? I don't > > remember how `LangAS::Default` works in OpenCL C++ enough to understand how > > it fits in here. > That's correct we should implement the same logic for the arguments too. I > will create a helper function or do you think we should just call > `CompareImplicitConversionSequence` on the method type too? > > I think `isAddressSpaceSupersetOf` can't be used here because it determines > compatibility of address spaces. However, the logic we are expressing is for > the address space preference instead. If I understand correctly, we already call `CompareImplicitConversionSequence` on the object-argument conversion above, as part of the `for (unsigned ArgIdx = StartArg; ArgIdx < NumArgs; ++ArgIdx) ` loop. I think the address-space ordering rule can be reasonably based on compatibility. In fact, I think it already is in our implementation. The right rule is basically that (1) an exact match is better than a conversion and (2) a conversion to a subspace is better than a conversion to a superspace. You can think of this as modifying the "proper subset" rule of [over.ics.rank]p3.2.5, which in implementation terms means `Qualifiers::isMoreQualifiedThan`, which already ends up using `isAddressSpaceSupersetOf`. So if this ranking isn't working right, it's probably because we're incorrectly fast-pathing based on CVR qualifiers somewhere, and in fact I can see a pretty suspicious check like that in `CompareQualificationConversions`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55850/new/ https://reviews.llvm.org/D55850 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
