[clang][NFC] Add IsAnyDestructorNoReturn field to CXXRecord instead of calculating it on demand

[zero9178]

Summary

This patch addresses a performance issue I noticed when using clang-12 to compile projects of mine. Even though the files weren't too large (around 1k cpp), the compiler was taking more than a minute to compile the source file, much longer than either GCC or MSVC.

Using a profiler it turned out the issue was the isAnyDestructorNoReturn function in CXXRecordDecl:

In particular it being recursive, recalculating the property for every invocation, for every field and base class. This showed up in tracebacks in the profiler as follows:

This patch instead adds IsAnyDestructorNoReturn as a Field to the data inside of CXXRecord and updates when a new base class, destructor, or record field member is added.

After this patch the problematic file of mine went from a compile time of 81s, down to 12s. The hotspots now look more normal as well:

The patch itself should not change any functionality, just improve performance.

[davrec]

Was this performance hit when using the static analyzer? A quick search suggests isAnyDestructorNoReturn() is only called within the analyzer, whereas comparable CXXRecordDecl methods whose results are stored (hasIrrelevantDestructor() etc.) seem to be called somewhere by Sema.

So non-users of the analyzer would not benefit from this change, and will incur a slight cost, IIUC. Is that cost remotely noticeable? Probably not, but a quick test along those lines would be helpful.

All in all this is probably good and advisable.

[zero9178]

In D104182#2815397, @davrec wrote:

Was this performance hit when using the static analyzer? A quick search suggests isAnyDestructorNoReturn() is only called within the analyzer, whereas comparable CXXRecordDecl methods whose results are stored (hasIrrelevantDestructor() etc.) seem to be called somewhere by Sema.

So non-users of the analyzer would not benefit from this change, and will incur a slight cost, IIUC. Is that cost remotely noticeable? Probably not, but a quick test along those lines would be helpful.

All in all this is probably good and advisable.

The only place where it is called in the static analyzer is in ExprEngineCXX.cpp:650. I was doing a normal compilation of a C++ file of mine, and they were coming from Analysis/CFG.cpp:1871 in addAutomaticObjDtors as well as CFG.cpp:4836 in VisitCXXBindTemporaryForDtors. So depending on the contents of the users C++ file it could improve their compilation speed as well. I suspect that in my case it was producing such a deep stacktrace due to a template class that is using a lot of layers of inheritance to preserve triviallity of constructors and more, tho that is just a guess. Nevertheless I'd wager it will more than likely improve the compile time for some other people as well.

As another test I just compared the compilation of X86ISelLowering.cpp from LLVM using clang-cl trunk, with and without this patch and could not measure any difference but margin of error. So it's definitely not guaranteed to be an improvement, but least isn't any worse.

clang/include/clang/AST/CXXRecordDeclDefinitionBits.def

 /// Whether any declared copy constructor has a const-qualified
 /// parameter.
 FIELD(HasDeclaredCopyConstructorWithConstParam, 1, MERGE_OR)
 
 /// Whether any declared copy assignment operator has either a
 /// const-qualified reference parameter or a non-reference parameter.
 FIELD(HasDeclaredCopyAssignmentWithConstParam, 1, MERGE_OR)
 
+/// Whether the destructor is no-return. Either explicitly, or if any
+/// base classes or fields have a no-return destructor
+FIELD(IsAnyDestructorNoReturn, 1, NO_MERGE)
+
 #undef FIELD

clang/include/clang/AST/DeclCXX.h

return const_cast<CXXRecordDecl *>(const_cast<const CXXRecordDecl *>(this)
                                            ->getTemplateInstantiationPattern());
   }
 
   /// Returns the destructor decl for this class.
   CXXDestructorDecl *getDestructor() const;
 
   /// Returns true if the class destructor, or any implicitly invoked
   /// destructors are marked noreturn.
-  bool isAnyDestructorNoReturn() const;
+  bool isAnyDestructorNoReturn() const { return data().IsAnyDestructorNoReturn; }
 
   /// If the class is a local class [class.local], returns
   /// the enclosing function declaration.
   const FunctionDecl *isLocalClass() const {
     if (const auto *RD = dyn_cast<CXXRecordDecl>(getDeclContext()))
       return RD->isLocalClass();
 
     return dyn_cast<FunctionDecl>(getDeclContext());

clang/lib/AST/DeclCXX.cpp

: UserDeclaredConstructor(false), UserDeclaredSpecialMembers(0),
       HasConstexprDefaultConstructor(false),
       DefaultedDestructorIsConstexpr(true),
       HasNonLiteralTypeFieldsOrBases(false), StructuralIfLiteral(true),
       UserProvidedDefaultConstructor(false), DeclaredSpecialMembers(0),
       ImplicitCopyConstructorCanHaveConstParamForVBase(true),
       ImplicitCopyConstructorCanHaveConstParamForNonVBase(true),
       ImplicitCopyAssignmentHasConstParam(true),
       HasDeclaredCopyConstructorWithConstParam(false),
-      HasDeclaredCopyAssignmentWithConstParam(false), IsLambda(false),
+      HasDeclaredCopyAssignmentWithConstParam(false),
+      IsAnyDestructorNoReturn(false), IsLambda(false),
       IsParsingBaseSpecifiers(false), ComputedVisibleConversions(false),
       HasODRHash(false), Definition(D) {}
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
   return Bases.get(Definition->getASTContext().getExternalSource());
 }
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getVBasesSlowCase() const {
if (!BaseClassDecl->hasTrivialDestructor())
       data().HasTrivialSpecialMembers &= ~SMF_Destructor;
 
     if (!BaseClassDecl->hasTrivialDestructorForCall())
       data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
 
     if (!BaseClassDecl->hasIrrelevantDestructor())
       data().HasIrrelevantDestructor = false;
 
+    if (BaseClassDecl->isAnyDestructorNoReturn())
+      data().IsAnyDestructorNoReturn = true;
+
     // C++11 [class.copy]p18:
     //   The implicitly-declared copy assignment operator for a class X will
     //   have the form 'X& X::operator=(const X&)' if each direct base class B
     //   of X has a copy assignment operator whose parameter is of type 'const
     //   B&', 'const volatile B&', or 'B' [...]
     if (!BaseClassDecl->hasCopyAssignmentWithConstParam())
       data().ImplicitCopyAssignmentHasConstParam = false;
 
if (const auto *DD = dyn_cast<CXXDestructorDecl>(D)) {
     // finishedDefaultedOrDeletedMember.
 
     // C++11 [class.dtor]p5:
     //   A destructor is trivial if [...] the destructor is not virtual.
     if (DD->isVirtual()) {
       data().HasTrivialSpecialMembers &= ~SMF_Destructor;
       data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
     }
+
+    if (DD->isNoReturn())
+      data().IsAnyDestructorNoReturn = true;
   }
 
   // Handle member functions.
   if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
     if (Method->isCopyAssignmentOperator()) {
       SMKind |= SMF_CopyAssignment;
 
       const auto *ParamTy =
if (const auto *RecordTy = T->getAs<RecordType>()) {
           data().HasTrivialSpecialMembers &= ~SMF_MoveAssignment;
 
         if (!FieldRec->hasTrivialDestructor())
           data().HasTrivialSpecialMembers &= ~SMF_Destructor;
         if (!FieldRec->hasTrivialDestructorForCall())
           data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
         if (!FieldRec->hasIrrelevantDestructor())
           data().HasIrrelevantDestructor = false;
+        if (FieldRec->isAnyDestructorNoReturn())
+          data().IsAnyDestructorNoReturn = true;
         if (FieldRec->hasObjectMember())
           setHasObjectMember(true);
         if (FieldRec->hasVolatileMember())
           setHasVolatileMember(true);
         if (FieldRec->getArgPassingRestrictions() ==
             RecordDecl::APK_CanNeverPassInRegs)
           setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs);
 
DeclarationName Name
     = Context.DeclarationNames.getCXXDestructorName(
                                           Context.getCanonicalType(ClassType));
 
   DeclContext::lookup_result R = lookup(Name);
 
   return R.empty() ? nullptr : dyn_cast<CXXDestructorDecl>(R.front());
 }
 
-bool CXXRecordDecl::isAnyDestructorNoReturn() const {
-  // Destructor is noreturn.
-  if (const CXXDestructorDecl *Destructor = getDestructor())
-    if (Destructor->isNoReturn())
-      return true;
-
-  // Check base classes destructor for noreturn.
-  for (const auto &Base : bases())
-    if (const CXXRecordDecl *RD = Base.getType()->getAsCXXRecordDecl())
-      if (RD->isAnyDestructorNoReturn())
-        return true;
-
-  // Check fields for noreturn.
-  for (const auto *Field : fields())
-    if (const CXXRecordDecl *RD =
-            Field->getType()->getBaseElementTypeUnsafe()->getAsCXXRecordDecl())
-      if (RD->isAnyDestructorNoReturn())
-        return true;
-
-  // All destructors are not noreturn.
-  return false;
-}
-
 static bool isDeclContextInNamespace(const DeclContext *DC) {
   while (!DC->isTranslationUnit()) {
     if (DC->isNamespace())
       return true;
     DC = DC->getParent();
   }
   return false;
 }