[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers
From:       John McCall via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2018-12-31 21:13:23
Message-ID: b03f3a696c3e38aee4e57fc1a9cd767c () localhost ! localdomain
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]