[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/AST/ItaniumMangle.cpp:1507
+    Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+        Method->getTypeQualifiers().getCVRUQualifiers());
     // We do not consider restrict a distinguishing attribute for overloading
----------------
mikael wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > You can overload based on the address space, right?  I think it needs to be mangled.
> > > Does this refer to our earlier discussion https://reviews.llvm.org/D54862#inline-484509
> > > 
> > > We don't have a way to qualify methods with an address space yet? I was going to send an RFC to `cfe-dev` for this but if you think it would be ok to go ahead with an implementation, I am happy with it. Either way would it be better to do this in a separate patch?
> > I'm fine with delaying implementation on these two issues until a later patch since, as you say, they can't be tested well until we support arbitrary address-space qualifiers.  Please at least leave FIXMEs for them.
> I actually did update this, since we can test the mangling of __generic in the test.
Great, thanks.  Looks good.


================
Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }
----------------
The indentation here is messed up.

You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few places.  That's currently correct, in the sense that the fast qualifiers are currently defined to be the CVR qualifiers, but the point of the distinction is that we might want to change that (and we have in fact explored that in the past).  I think you should make `FunctionTypeBits` only claim to store the fast qualifiers, which mostly just means updating a few field / accessor names and changing things like the `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.

If there are places where it's convenient to assume that the CVR qualifiers are exactly the fast qualifiers, it's okay to static_assert that, but you should make sure to static_assert it in each place you assume it.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+      getOrCreateInstanceMethodType(
+          CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+          FPT, U),
----------------
Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?


================
Comment at: lib/CodeGen/CGDeclCXX.cpp:32
+       (D.hasLocalStorage() && CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+      "VarDecl must have global or local (in the case of OpenCL) storage!");
   assert(!D.getType()->isReferenceType() &&
----------------
What causes OpenCL C++ declarations to go down this path?


================
Comment at: lib/Index/USRGeneration.cpp:274
+    if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
       Out << (char)('0' + quals);
     switch (MD->getRefQualifier()) {
----------------
akyrtzi wrote:
> rjmccall wrote:
> > Paging @akyrtzi here.  The address-space qualifier should be part of the USR but I don't think if there's a defined schema for that.
> If I understand correctly from other comments you can't add special mangling and USR generation yet and intend to add FIXME for doing mangling support later ? If yes, could you also add FIXME here and re-visit ?
While you're here, can you described the right way for us to extend USR generation here to potentially add an address space?


================
Comment at: lib/Parse/ParseDecl.cpp:6166
+
+      Qualifiers Q = Qualifiers::fromOpaqueValue(DS.getTypeQualifiers());
+      if (D.getDeclSpec().isConstexprSpecified() && !getLangOpts().CPlusPlus14)
----------------
I think `fromCVRUQualifiers` is probably the right function, and if that doesn't exist you should add it.


================
Comment at: lib/Sema/SemaExprCXX.cpp:1112
+  QualType T = S.Context.getRecordType(Record).withCVRQualifiers(Quals);
+  T = S.getASTContext().getAddrSpaceQualType(T, CXXThisTypeQuals.getAddressSpace());
+
----------------
I don't think there's a good reason for this to be dropping non-CVR qualifiers.  If there is, it really needs a comment.


================
Comment at: lib/Sema/TreeTransform.h:5276
+      Sema::CXXThisScopeRAII ThisScope(
+          SemaRef, ThisContext, Qualifiers::fromOpaqueValue(ThisTypeQuals));
 
----------------
I think you need to turn `ThisTypeQuals` into a `Qualifiers` here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862