[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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