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

[Markus Böck via cfe-commits]

Author: Markus Böck
Date: 2021-06-13T14:48:27+02:00
New Revision: 7ff3a89a7b94193638cb13f8a0a1ef70094c8263

URL: https://github.com/llvm/llvm-project/commit/7ff3a89a7b94193638cb13f8a0a1ef70094c8263
DIFF: https://github.com/llvm/llvm-project/commit/7ff3a89a7b94193638cb13f8a0a1ef70094c8263.diff

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

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.

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 patch itself should not change any functionality, just improve performance.

Differential Revision: https://reviews.llvm.org/D104182

Added: 
    

Modified: 
    clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
    clang/include/clang/AST/DeclCXX.h
    clang/lib/AST/DeclCXX.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
index d15d6698860f4..9b270682f8cf0 100644
--- a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
+++ b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
@@ -242,4 +242,8 @@ FIELD(HasDeclaredCopyConstructorWithConstParam, 1, MERGE_OR)
 /// 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

diff  --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index e9f9da6bd4bc4..0d5ad40fc19e7 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1480,7 +1480,7 @@ class CXXRecordDecl : public RecordDecl {
 
   /// 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.

diff  --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index bcff72ccadeab..aeee35d9c74f6 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -108,7 +108,8 @@ CXXRecordDecl::DefinitionData::DefinitionData(CXXRecordDecl *D)
       ImplicitCopyConstructorCanHaveConstParamForNonVBase(true),
       ImplicitCopyAssignmentHasConstParam(true),
       HasDeclaredCopyConstructorWithConstParam(false),
-      HasDeclaredCopyAssignmentWithConstParam(false), IsLambda(false),
+      HasDeclaredCopyAssignmentWithConstParam(false),
+      IsAnyDestructorNoReturn(false), IsLambda(false),
       IsParsingBaseSpecifiers(false), ComputedVisibleConversions(false),
       HasODRHash(false), Definition(D) {}
 
@@ -424,6 +425,9 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases,
     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
@@ -836,6 +840,9 @@ void CXXRecordDecl::addedMember(Decl *D) {
       data().HasTrivialSpecialMembers &= ~SMF_Destructor;
       data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
     }
+
+    if (DD->isNoReturn())
+      data().IsAnyDestructorNoReturn = true;
   }
 
   // Handle member functions.
@@ -1233,6 +1240,8 @@ void CXXRecordDecl::addedMember(Decl *D) {
           data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
         if (!FieldRec->hasIrrelevantDestructor())
           data().HasIrrelevantDestructor = false;
+        if (FieldRec->isAnyDestructorNoReturn())
+          data().IsAnyDestructorNoReturn = true;
         if (FieldRec->hasObjectMember())
           setHasObjectMember(true);
         if (FieldRec->hasVolatileMember())
@@ -1888,29 +1897,6 @@ CXXDestructorDecl *CXXRecordDecl::getDestructor() const {
   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())