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

[Markus Böck via Phabricator via cfe-commits]

zero9178 created this revision.
zero9178 added reviewers: davrec, bruno, rsmith, aaron.ballman.
zero9178 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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:
F17356915: image.png <https://reviews.llvm.org/F17356915>

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:
F17356931: image.png <https://reviews.llvm.org/F17356931>

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:
F17356944: image.png <https://reviews.llvm.org/F17356944>

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104182

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


Index: clang/lib/AST/DeclCXX.cpp
===================================================================
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -108,7 +108,8 @@
       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 @@
     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 @@
       data().HasTrivialSpecialMembers &= ~SMF_Destructor;
       data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
     }
+
+    if (DD->isNoReturn())
+      data().IsAnyDestructorNoReturn = true;
   }
 
   // Handle member functions.
@@ -1233,6 +1240,8 @@
           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 @@
   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())
Index: clang/include/clang/AST/DeclCXX.h
===================================================================
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -1480,7 +1480,7 @@
 
   /// 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.
Index: clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
===================================================================
--- clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
+++ clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
@@ -242,4 +242,8 @@
 /// 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104182.351661.patch
Type: text/x-patch
Size: 3861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210612/d31f87dc/attachment.bin>