[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added reviewers: Anastasia, rjmccall.
mantognini marked 6 inline comments as done.
mantognini added inline comments.


================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
-                            /*Delegating=*/false, addr);
+                            /*Delegating=*/false, addr, type);
 }
----------------
This is the only place where the new parameter has an interesting value. In the ~10 other calls to `EmitCXXDestructorCall` overloads, the default value of this new parameter is used instead.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:96
+    llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
+    QualType ThisTy) {
+  const CXXMethodDecl *DtorDecl = cast<CXXMethodDecl>(Dtor.getDecl());
----------------
This new parameter is required in order to call `performAddrSpaceCast`.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+      llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+      This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+                                                   NewType);
+    }
----------------
This is effectively the fix for the third point mentioned in the description.

Alternatively, `This = Builder.CreatePointerBitCastOrAddrSpaceCast(This, NewType);` seems to work equally well and does not require the extra new parameter.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8428-8439
+    bool DiagOccured = false;
     FTI.MethodQualifiers->forEachQualifier(
         [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
+          // This diagnostic should be emitted on any qualifier except an addr
+          // space qualifier. However, forEachQualifier currently doesn't visit
+          // addr space qualifiers, so there's no way to write this condition
+          // right now; we just diagnose on everything.
----------------
This is the fix for the first point in the description.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:5092-5097
-  Qualifiers Quals;
+  Qualifiers Quals = Method->getMethodQualifiers();
   if (isa<CXXDestructorDecl>(Method)) {
     Quals.addConst();
     Quals.addVolatile();
-  } else {
-    Quals = Method->getMethodQualifiers();
----------------
This is the fix for the second point in the description.


================
Comment at: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl:1
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
----------------
This test extends `addrspace-ctor.cl`, which is therefore deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569