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

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7ff3a89a7b94: [clang][NFC] Add IsAnyDestructorNoReturn field 
to CXXRecord instead of… (authored by zero9178).

Repository:
  rG LLVM Github Monorepo

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

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(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  : 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


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),
   

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

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

zero9178 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104182

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104182

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 

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 

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 

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(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  : 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
+++