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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/AST/DeclCXX.cpp:2187
   ClassTy = C.getQualifiedType(ClassTy,
                                Qualifiers::fromCVRUMask(getTypeQualifiers()));
+
----------------
I feel like the right design now is for `getTypeQualifiers()` to return a `Qualifiers` and not just a mask.

That will also help identify other places that need to be updated to support address-space qualification.


================
Comment at: lib/CodeGen/CGCall.cpp:71
 /// Derives the 'this' type for codegen purposes, i.e. ignoring method
 /// qualification.
+static CanQualType GetThisType(ASTContext &Context, const CXXRecordDecl *RD, const CXXMethodDecl *MD) {
----------------
Please update the comment to say "ignoring method CVR qualification" instead.

Also, maybe we should just eliminate this and use `CXXMethodDecl::getThisType`.  If we want to preserve the micro-optimization of sharing `CGFunctionInfo`s for method types that only differ by CV qualification, maybe we should add a parameter to `getThisType` to not add ABI-compatible qualification.


================
Comment at: lib/CodeGen/CGClass.cpp:2020
+    unsigned TargetAS = getContext().getTargetAddressSpace(AS);
+    llvm::Type *NewType = ThisPtr->getType()->getPointerTo(TargetAS);
+    ThisPtr = getTargetHooks().performAddrSpaceCast(
----------------
This is adding an extra level of pointer, I think.


================
Comment at: lib/CodeGen/CGClass.cpp:2023
+        *this, This.getPointer(),
+        getLangASFromTargetAS(This.getAddressSpace()), AS,
+        NewType);
----------------
...I'm not sure when `getLangASFromTargetAS` was added, but it shouldn't exist; you need to pass down the language address space of `This`, which in general will come from the pointer type.  And then the `AS != LangAS::Default` check should just be comparing the constructor's address space to that address space, not specifically `LangAS::Default`.

Same comments apply to the code in `commonEmitCXXMemberOrOperatorCall`.


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

https://reviews.llvm.org/D54862