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

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked 10 inline comments as done.
mantognini added a comment.

Mind the fact that I've rebased the changes onto a more recent master version. If you look at the diff of v1 against v2 you might see some unrelated changes.

Let me know if there's anything else that I need to change.



================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
-                            /*Delegating=*/false, addr);
+                            /*Delegating=*/false, addr, type);
 }
----------------
rjmccall wrote:
> 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.
I've addressed that. I had to add some extra parameter/attributes here and there. Please let me know if anything is can be improved.


================
Comment at: clang/lib/CodeGen/CGClass.cpp:1447
+    if (HaveInsertPoint()) {
+      // FIXME: Determine the type of the object being destroyed.
+      QualType ThisTy;
----------------
I'm not familiar enough with CXXCtorType and such, and would welcome some help for these two FIXMEs.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+  // we ensure a cast is added where necessary.
+  if (ThisTy.isNull()) {
+#ifndef NDEBUG
----------------
Despite no longer having a default parameter, not all call site can provide a meaningful value ATM. That is why this check is still required.


================
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());
----------------
mantognini wrote:
> This new parameter is required in order to call `performAddrSpaceCast`.
Now that it's no longer a default parameter. I've group ThisTy with This.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+      llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+      This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+                                                   NewType);
+    }
----------------
rjmccall wrote:
> 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.

Alright, I've kept it.


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


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8197
+  const DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
+  if (FTI.hasMethodTypeQualifiers() && !D.isInvalidType()) {
+    bool DiagOccured = false;
----------------
I've refactored that bit out of the two mentioned functions. Mind the fact that it now always check `!D.isInvalidType()`. I think it makes sense, but let me know if that's not the case.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8200
+    FTI.MethodQualifiers->forEachQualifier(
+        [DiagID, &S, &DiagOccured](DeclSpec::TQ, StringRef QualName,
+                                   SourceLocation SL) {
----------------
I've reduced the capture group to the bare minimum. Hopefully, this is consistent with LLVM style. But let me know if I should change this back to `[&]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569