[clang-tidy] Improve performance of google-runtime-int #86596
Conversation
Main problem with performance of this check is caused by hasAncestor matcher, and to be more precise by an llvm::DenseSet and std::deque in matchesAnyAncestorOf. To reduce impact of this matcher, multiple conditions that were checked in check method were copied into ast matcher that is now checked before hasAncestor. Using custom getCheckTraversalKind to exclude template instances that shouldn't be checked anyway is an additional improvment, but gain from that one is low. Tested on ffl_tests.cc, visible reduction from ~442 seconds to ~15 seconds (~96% reduction). Closes llvm#86553
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Piotr Zegar (PiotrZSL) ChangesMain problem with performance of this check is caused by hasAncestor matcher, and to be more precise by an llvm::DenseSet and std::deque in matchesAnyAncestorOf. To reduce impact of this matcher, multiple conditions that were checked in check method were copied into AST matcher that is now checked before hasAncestor. Using custom getCheckTraversalKind to exclude template instances that shouldn't be checked anyway is an additional improvement, but gain from that one is low. Tested on ffl_tests.cc, visible reduction from ~442 seconds to ~15 seconds (~96% reduction). Closes #86553 Full diff: https://github.com/llvm/llvm-project/pull/86596.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp b/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp
index ef511e9108f2ee..359d8efd100bad 100644
--- a/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp
@@ -40,6 +40,34 @@ namespace {
AST_MATCHER(FunctionDecl, isUserDefineLiteral) {
return Node.getLiteralIdentifier() != nullptr;
}
+
+AST_MATCHER(TypeLoc, isValidAndNotInMacro) {
+ const SourceLocation Loc = Node.getBeginLoc();
+ return Loc.isValid() && !Loc.isMacroID();
+}
+
+AST_MATCHER(TypeLoc, isBuiltinType) {
+ TypeLoc TL = Node;
+ if (auto QualLoc = Node.getAs<QualifiedTypeLoc>())
+ TL = QualLoc.getUnqualifiedLoc();
+
+ const auto BuiltinLoc = TL.getAs<BuiltinTypeLoc>();
+ if (!BuiltinLoc)
+ return false;
+
+ switch (BuiltinLoc.getTypePtr()->getKind()) {
+ case BuiltinType::Short:
+ case BuiltinType::Long:
+ case BuiltinType::LongLong:
+ case BuiltinType::UShort:
+ case BuiltinType::ULong:
+ case BuiltinType::ULongLong:
+ return true;
+ default:
+ return false;
+ }
+}
+
} // namespace
namespace tidy::google::runtime {
@@ -63,11 +91,11 @@ void IntegerTypesCheck::registerMatchers(MatchFinder *Finder) {
// "Where possible, avoid passing arguments of types specified by
// bitwidth typedefs to printf-based APIs."
Finder->addMatcher(
- typeLoc(loc(isInteger()),
- unless(anyOf(hasAncestor(callExpr(
- callee(functionDecl(hasAttr(attr::Format))))),
- hasParent(parmVarDecl(hasAncestor(
- functionDecl(isUserDefineLiteral())))))))
+ typeLoc(loc(isInteger()), isValidAndNotInMacro(), isBuiltinType(),
+ unless(hasAncestor(
+ callExpr(callee(functionDecl(hasAttr(attr::Format)))))),
+ unless(hasParent(parmVarDecl(
+ hasAncestor(functionDecl(isUserDefineLiteral()))))))
.bind("tl"),
this);
IdentTable = std::make_unique<IdentifierTable>(getLangOpts());
@@ -77,9 +105,6 @@ void IntegerTypesCheck::check(const MatchFinder::MatchResult &Result) {
auto TL = *Result.Nodes.getNodeAs<TypeLoc>("tl");
SourceLocation Loc = TL.getBeginLoc();
- if (Loc.isInvalid() || Loc.isMacroID())
- return;
-
// Look through qualification.
if (auto QualLoc = TL.getAs<QualifiedTypeLoc>())
TL = QualLoc.getUnqualifiedLoc();
diff --git a/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.h b/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.h
index 3be7d24021b9ff..c62bda67ae2d98 100644
--- a/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.h
+++ b/clang-tools-extra/clang-tidy/google/IntegerTypesCheck.h
@@ -35,6 +35,9 @@ class IntegerTypesCheck : public ClangTidyCheck {
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
private:
const StringRef UnsignedTypePrefix;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a604e9276668ae..4d1cd57c2fd478 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -197,6 +197,9 @@ Changes in existing checks
<clang-tidy/checks/google/global-names-in-headers>` check by replacing the local
option `HeaderFileExtensions` by the global option of the same name.
+- Improved :doc:`google-runtime-int <clang-tidy/checks/google/runtime-int>`
+ check performance through optimizations.
+
- Improved :doc:`llvm-header-guard
<clang-tidy/checks/llvm/header-guard>` check by replacing the local
option `HeaderFileExtensions` by the global option of the same name.
|
|
Well, its down to about 15 minutes from 1hr 40 min, which is a pretty great improvement. 15 minutes does still seem to be too long for a file that's only 2KLOC. I know its more complex w/ headers and macros, but it does only take about 1-2 minutes to compile normally, so I feel like it should finish a bit faster than what I'm currently seeing with this patch. But overall, I'm pretty thrilled to see this massive improvement!. Thanks. |
|
I will go with merging this. There are still few improvements possible, but they require partial check rewrite. |
Main problem with performance of this check is caused by hasAncestor matcher, and to be more precise by an llvm::DenseSet and std::deque in matchesAnyAncestorOf.
To reduce impact of this matcher, multiple conditions that were checked in check method were copied into AST matcher that is now checked before hasAncestor.
Using custom getCheckTraversalKind to exclude template instances that shouldn't be checked anyway is an additional improvement, but gain from that one is low.
Tested on ffl_tests.cc, visible reduction from ~442 seconds to ~15 seconds (~96% reduction).
Closes #86553