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

[John McCall via Phabricator via cfe-commits]

rjmccall 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);
 }
----------------
mantognini wrote:
> 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.
Arguments that are potentially required for correctness — as opposed to just enabling some optimization — should generally not have defaults.  I think you should remove these defaults and require a sensible type to be passed down in all cases.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+      llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+      This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+                                                   NewType);
+    }
----------------
Anastasia wrote:
> mantognini wrote:
> > 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.
> Yes, I agree just using `CreatePointerBitCastOrAddrSpaceCast` would be easier.
> 
> As far as I remember (@rjmccall can correct me if I am wrong) `performAddrSpaceCast` was added to workaround the fact that `nullptr` constant might not be value 0 for some address spaces. However, considering that it's not the first place where some big change has to be done in CodeGen to be able to use the new API I am wondering if it's worth looking at moving this logic to LLVM lowering phases that can map `nullptr` constant into some arbitrary value. I think the challenge is to find where LLVM assumes that `nullptr` is always 0. That might not be an easy task.
> 
> PS, I am not suggesting to do it for this patch but just an idea to investigate in the future. 
I continue to think that it makes sense to set Clang up to be able to handle language-level address spaces that don't exist at the level of LLVM IR.


================
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.
----------------
mantognini wrote:
> This is the fix for the first point in the description.
Can we unify this with the similar check we do elsewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569