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

[John McCall via Phabricator via cfe-commits]

rjmccall added a subscriber: akyrtzi.
rjmccall added a comment.

I have a lot of comments about various things that need to be fixed, but I just want to take a moment to say that this is a tremendously impressive patch: given only some very high-level directions, you've done a great job figuring out what you needed to do to carry that out and make it work.  Really well done.



================
Comment at: lib/AST/ASTDumper.cpp:346
+        break;
+      }
       switch (EPI.RefQualifier) {
----------------
We must have some existing code to print an address space.


================
Comment at: lib/AST/ItaniumMangle.cpp:1507
+    Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+        Method->getTypeQualifiers().getCVRUQualifiers());
     // We do not consider restrict a distinguishing attribute for overloading
----------------
You can overload based on the address space, right?  I think it needs to be mangled.


================
Comment at: lib/AST/TypePrinter.cpp:804
 
-  if (unsigned quals = T->getTypeQuals()) {
+  if (unsigned quals = T->getTypeQuals().getCVRUQualifiers()) {
     OS << ' ';
----------------
You should be printing all the qualifiers here, including address spaces.  If you want to suppress an implicit OpenCL address-space qualifier, that seems reasonable.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
       getOrCreateInstanceMethodType(CGM.getContext().getPointerType(QualType(
-                                        Ty->getClass(), FPT->getTypeQuals())),
+                                        Ty->getClass(), FPT->getTypeQuals().getCVRUQualifiers())),
                                     FPT, U),
----------------
I think this is just `GetThisType` again.  Maybe there should be a version of that takes a record and an FPT; you could make it a static method on `CXXMethodDecl`.  At any rate, I don't think you shouldn't be dropping the address space.


================
Comment at: lib/Index/USRGeneration.cpp:274
+    if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
       Out << (char)('0' + quals);
     switch (MD->getRefQualifier()) {
----------------
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.


================
Comment at: lib/Parse/ParseCXXInlineMethods.cpp:419
     Sema::CXXThisScopeRAII ThisScope(Actions, Method->getParent(),
-                                     Method->getTypeQualifiers(),
+                                     Method->getTypeQualifiers().getCVRUQualifiers(),
                                      getLangOpts().CPlusPlus11);
----------------
This should take the full qualifiers.  You can see how it ends up getting reassembled into the `this` type in the `CXXThisScopeRAII` ctor, and that type definitely needs to be address-space-qualified.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:1046
+        Qualifiers MethodQuals = Qualifiers::fromCVRMask(
+            Method->getTypeQualifiers().getCVRQualifiers());
         if (ObjectTypeQualifiers == MethodQuals)
----------------
Hmm.  This is actually going to break code completion for these methods, I think: IIUC in OpenCL every pointer and l-value is address-space-qualified, so only considering the CVRU qualifiers means that there will always be "extra" qualifiers on the pointer and code completion will think it doesn't match.

...I know code completion isn't your top priority, but this is probably easy enough to fix.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:3701
+      Results.setObjectTypeQualifiers(Qualifiers::fromCVRMask(
+          CurMethod->getTypeQualifiers().getCVRQualifiers()));
       MemberCompletionRecord = CurMethod->getParent();
----------------
This seems to be intentionally dropping non-CVR qualifiers, and I don't know why you ever would want to do that.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:11963
+            BaseType,
+            CopyAssignOperator->getTypeQualifiers().getCVRUQualifiers()),
+        VK_LValue, BasePath);
----------------
This should definitely not be dropping address spaces.  You'll need to make a second patch to handle special members correctly, so you don't need to add a test for this yet, but you might as well get the logic right right now.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:12332
+            BaseType,
+            MoveAssignOperator->getTypeQualifiers().getCVRQualifiers()),
+        VK_LValue, BasePath);
----------------
Same thing as above.


================
Comment at: lib/Sema/SemaOverload.cpp:1146
+    unsigned OldQuals = OldMethod->getTypeQualifiers().getCVRUQualifiers();
+    unsigned NewQuals = NewMethod->getTypeQualifiers().getCVRUQualifiers();
     if (!getLangOpts().CPlusPlus14 && NewMethod->isConstexpr() &&
----------------
This is an algorithm that I think you need to get right and which shouldn't drop address spaces.


================
Comment at: lib/Sema/SemaOverload.cpp:2827
+  unsigned FromQuals = FromFunction->getTypeQuals().getCVRUQualifiers();
+  unsigned ToQuals = ToFunction->getTypeQuals().getCVRUQualifiers();
   if (FromQuals != ToQuals) {
----------------
This is fair to punt to a follow-up patch, but you *should* make that patch.


================
Comment at: lib/Sema/SemaOverload.cpp:5070
+                       ? Qualifiers::Const | Qualifiers::Volatile
+                       : Method->getTypeQualifiers().getCVRUQualifiers();
   QualType ImplicitParamType =  S.Context.getCVRQualifiedType(ClassType, Quals);
----------------
You should be using the full qualifiers here.


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:4660
+      ArgTy,
+      Qualifiers::fromCVRMask(Method->getTypeQualifiers().getCVRQualifiers()));
   if (Method->getRefQualifier() == RQ_RValue)
----------------
Again, this is dropping non-CVR qualifiers for no particular reason.


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

https://reviews.llvm.org/D54862