This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Improve destructor support in C++ for OpenCL
ClosedPublic

Authored by mantognini on Jul 11 2019, 9:15 AM.

Details

Summary

This patch does mainly three things:

  1. It fixes a false positive error detection in Sema that is similar to D62156. The error happens when explicitly calling an overloaded destructor for different address spaces.
  2. It selects the correct destructor when multiple overloads for address spaces are available.
  3. It inserts the expected address space cast when invoking a destructor, if needed, and therefore fixes a crash due to the unmet assertion in llvm::CastInst::Create.

The following is a reproducer of the three issues:

struct MyType {
  ~MyType() {}
  ~MyType() __constant {}
};

__constant MyType myGlobal{};

kernel void foo() {
  myGlobal.~MyType(); // 1 and 2.
  // 1. error: cannot initialize object parameter of type
  //    '__generic MyType' with an expression of type '__constant MyType'
  // 2. error: no matching member function for call to '~MyType'
}

kernel void bar() {
  // 3. The implicit call to the destructor crashes due to:
  //    Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
  //    in llvm::CastInst::Create.
  MyType myLocal;
}

The added test depends on D62413 and covers a few more things than the
above reproducer.

Event Timeline

mantognini created this revision.Jul 11 2019, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 9:15 AM
mantognini marked 6 inline comments as done.
mantognini added inline comments.
clang/lib/CodeGen/CGClass.cpp
2022

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.

clang/lib/CodeGen/CGExprCXX.cpp
96

This new parameter is required in order to call performAddrSpaceCast.

117–118

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.

clang/lib/Sema/SemaDeclCXX.cpp
8433–8444

This is the fix for the first point in the description.

clang/lib/Sema/SemaOverload.cpp
5092–5097

This is the fix for the second point in the description.

clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
2

This test extends addrspace-ctor.cl, which is therefore deleted.

Anastasia added inline comments.Jul 12 2019, 4:53 AM
clang/lib/CodeGen/CGExprCXX.cpp
117–118

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.

rjmccall added inline comments.Jul 12 2019, 12:29 PM
clang/lib/CodeGen/CGClass.cpp
2022

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.

clang/lib/CodeGen/CGExprCXX.cpp
117–118

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.

clang/lib/Sema/SemaDeclCXX.cpp
8433–8444

Can we unify this with the similar check we do elsewhere?

  • Refactored common bits from CheckConstructorDeclarator and CheckDestructorDeclarator.
  • Added as many "ThisTy" parameter I could.
  • Addressed issue with identifier format in tests (%N to %var.ascast).
mantognini marked 10 inline comments as done.Jul 16 2019, 6:26 AM

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.

clang/lib/CodeGen/CGClass.cpp
1447

I'm not familiar enough with CXXCtorType and such, and would welcome some help for these two FIXMEs.

2022

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.

clang/lib/CodeGen/CGExprCXX.cpp
96

Now that it's no longer a default parameter. I've group ThisTy with This.

103

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.

117–118

Alright, I've kept it.

clang/lib/Sema/SemaDeclCXX.cpp
8197

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.

8200

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 [&].

8433–8444

Done.

rjmccall added inline comments.Jul 16 2019, 10:45 AM
clang/lib/CodeGen/CGClass.cpp
496

I think the rule we want is that the type passed here is the (qualified) object type, but getThisType() will return a pointer type. Consider adding a getThisObjectType() method to CXXMethodDecl which does that computation (and then make getThisType() just wrap that in a PointerType).

1447

Dtor->getThisObjectType() should be right in both of these cases.

2022

Thanks, other than the FIXMEs and the object/pointer mismatches this all looks right; very nicely done.

2374

Same thing about getThisObjectType().

clang/lib/CodeGen/CGExprCXX.cpp
103

Is that resolved with fixing the FIXME?

Please assert that ThisTy->getAsCXXRecordDecl() == Dtor->getParent() to guard against that pointer/object mixup.

364

ThisTy here can be either Base->getType() or Base->getType()->getPointeeType() depending on whether this is an arrow access.

clang/lib/CodeGen/ItaniumCXXABI.cpp
1760

It looks like the only places that pass down a null CE are the two implementations in emitVirtualObjectDelete, which both have a CXXDeleteExpr. You could just have this method take an llvm::PointerUnion<CXXMethodCallExpr*,CXXDeleteExpr*> or something, and then someone else could take advantage of that for better source locations on the virtual call eventually.

clang/lib/CodeGen/MicrosoftCXXABI.cpp
1929

Same point as in the ItaniumCXXABI implementation.

mantognini marked 16 inline comments as done.
  • Addressed comments
mantognini marked an inline comment as done.Jul 17 2019, 8:46 AM
mantognini added inline comments.
clang/lib/CodeGen/CGClass.cpp
496

I've done the suggested refactoring, let me know if it needs adjustment.

clang/lib/CodeGen/CGExprCXX.cpp
99

I wasn't 100% sure if this was covered by the next assertion.

103

Yes, indeed. I've therefore simplified the code and added the requested assertion.

364

Thanks for mentioning it. It should be better now.

clang/lib/CodeGen/ItaniumCXXABI.cpp
1760

Alright, makes sense. I've changed the function to take a PointerUnion and removed the FIXMEs.

LGTM with one minor request.

clang/lib/AST/DeclCXX.cpp
2265–2272

Thanks. Can you extract this implementation out into a static function (i.e. an internal linkage function in this file) that takes an ASTContext& so that getThisType doesn't have to fetch the ASTContext twice?

clang/lib/CodeGen/CGExprCXX.cpp
99

It is.

mantognini marked 4 inline comments as done.
  • Minor refactoring of getThisObjectType
  • Removed unnecessary assertion
mantognini added inline comments.Jul 18 2019, 1:54 AM
clang/lib/AST/DeclCXX.cpp
2265–2272

Sure, no problem.

clang/lib/CodeGen/CGExprCXX.cpp
99

Alright, thanks for the clarification. I've removed it as it doesn't bring anything.

Anastasia accepted this revision.Jul 18 2019, 2:54 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Jul 18 2019, 2:54 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 3:05 AM

The commit has been reverted due to issues with libc++:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190715/279833.html

However, without this change dtors are largely not usable in OpenCL due to ICE on wrong IR. I would like to be able to fix this on the release branch since we will publicize C++ for OpenCL as a feature.

I suggest to reopening this review and rework the patch.

mantognini reopened this revision.Jul 18 2019, 9:38 AM
mantognini added a subscriber: ilya-biryukov.

While investigating PR42665, I've noticed that getImplicitObjectArgument doesn't always return a pointer type. For example, when dealing with shared_ptr. In libc++'s test std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp, dumping CE in EmitVirtualDestructorCall shows that the callee is .~Bar (mind the .):

CXXMemberCallExpr 0x29d10a0 'void'
`-MemberExpr 0x29d1070 '<bound member function type>' .~Bar 0x23f5250
  `-CXXMemberCallExpr 0x29d1040 'struct Bar':'struct Bar' lvalue
    `-MemberExpr 0x29d1010 '<bound member function type>' .second 0x2883328
      `-MemberExpr 0x29d0f80 '__compressed_pair<class std::__1::allocator<struct Bar>, struct Bar>':'class std::__1::__compressed_pair<class std::__1::allocator<struct Bar>, struct Bar>' lvalue ->__data_ 0x2896960
        `-CXXThisExpr 0x29d0f70 'class std::__1::__shared_ptr_emplace<struct Bar, class std::__1::allocator<struct Bar> > *' implicit this

However, for CodeGenCXX/virtual-pseudo-destructor-call.cpp in clang test suite, CE is this (mind the -> this time):

CXXMemberCallExpr 0x7787a8 'void'
`-MemberExpr 0x778750 '<bound member function type>' ->~A 0x778288
  `-ImplicitCastExpr 0x778738 'struct A *' <LValueToRValue>
    `-DeclRefExpr 0x778708 'struct A *' lvalue ParmVar 0x778558 'a' 'struct A *'

Tomorrow, I'll try to reduce libc++ failing tests and add it to Clang's suite as part of this review. For now, I've run check-all and it seems that no test are failing anymore (with the patch I'm about to upload).

However, I'm not sure whether getImplicitObjectArgument is expected to have either pointer or non-pointer type. I guess it is, but, John, Ilya, could either of you confirm this?

This revision is now accepted and ready to land.Jul 18 2019, 9:38 AM

Add patch for Bug PR42665.

mantognini requested review of this revision.Jul 18 2019, 9:40 AM
mantognini marked an inline comment as done.Jul 18 2019, 9:47 AM
mantognini added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
1758–1764

This is the fix for PR42665. I've just realised that the same should be done in MicrosoftCXXABI.cpp -- will do that tomorrow.

Yes, that's the right fix, although you might also consider adding a getObjectType() to CXXMemberCallExpr.

  • Add minimal regression test for PR42665
  • Add CXXMemberCallExpr::getObjectType()

Yes, that's the right fix, although you might also consider adding a getObjectType() to CXXMemberCallExpr.

Thanks, John, it should be better now, but let me know if I can improve something.

This revision is now accepted and ready to land.Jul 19 2019, 9:33 AM

FYI In order to fix buildbot test failures, I've pushed https://reviews.llvm.org/rG1b2da771f561affe36eb5eb0c7a3d2862c5a5c1c. I'll keep an eye on buildbots for additional fallout.

clang/lib/Sema/SemaOverload.cpp