Anastasia marked 3 inline comments as done. Anastasia added inline comments.
================ Comment at: lib/Sema/SemaOverload.cpp:9279 + (CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default)) + return true; + } ---------------- rjmccall wrote: > 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`. I created a separate review to fix this: https://reviews.llvm.org/D56735 ================ Comment at: lib/Sema/SemaType.cpp:4877 T = Context.getFunctionType(T, ParamTys, EPI); T = state.getSema().Context.getAddrSpaceQualType(T, AS); } else { ---------------- brunodf wrote: > I follow all of the above (from the point "a class member function has an > address space"), but then I take issue with this line (already from Mikael). > > You look at the declaration's attributes, to derive the ASIdx relating to the > method's this argument. You mark the relevant attributes as invalid, to > prevent them from being considered in "processTypeAttrs" after the switch > that we break below. The ASIdx is stored in the qualifiers EPI to go to the > FunctionProtoType (this will affect getThisType). This all seems fine. > > But then this line adds the address space qualification to T, the type of the > declared function itself. This seems unnecessary, and conceptually wrong: > while the function may have an address space in syntax, this address space > applies to the this argument, not to the function object (and a pointer to > the function is not a pointer to this address space etc.). In short, I think > this line should be removed. I think I fixed it in: https://reviews.llvm.org/D56066 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
