Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] Improve performance of misc-const-correctness #72705

Conversation

PiotrZSL
Copy link
Member

Replaced certain AST matchers in ExprMutationAnalyzer with a more direct utilization of AST classes. The primary bottleneck was identified in the canResolveToExpr AST matcher. Since this matcher was employed multiple times and used recursively, each invocation led to the constant creation and destruction of other matchers within it. Additionally, the continual comparison of DynTypedNode resulted in significant performance degradation.

The optimization was tested on the TargetLowering.cpp file. Originally, the check took 156 seconds on that file, but after implementing this enhancement, it now takes approximately 40 seconds, making it nearly four times faster.

Despite this improvement, there are still numerous issues in this file. To further reduce the computational cost of this class, it is advisable to consider removing the remaining matchers and exploring alternatives such as leveraging RecursiveASTVisitor and increasing the direct use of AST classes.

Closes #71786

@PiotrZSL PiotrZSL self-assigned this Nov 17, 2023
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:analysis labels Nov 17, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 17, 2023

@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Changes

Replaced certain AST matchers in ExprMutationAnalyzer with a more direct utilization of AST classes. The primary bottleneck was identified in the canResolveToExpr AST matcher. Since this matcher was employed multiple times and used recursively, each invocation led to the constant creation and destruction of other matchers within it. Additionally, the continual comparison of DynTypedNode resulted in significant performance degradation.

The optimization was tested on the TargetLowering.cpp file. Originally, the check took 156 seconds on that file, but after implementing this enhancement, it now takes approximately 40 seconds, making it nearly four times faster.

Despite this improvement, there are still numerous issues in this file. To further reduce the computational cost of this class, it is advisable to consider removing the remaining matchers and exploring alternatives such as leveraging RecursiveASTVisitor and increasing the direct use of AST classes.

Closes #71786


Patch is 26.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/72705.diff

2 Files Affected:

  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1)
  • (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+186-164)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 23111be4371e2e1..eae460e7936ffd7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -326,7 +326,8 @@ Changes in existing checks
   <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when
   using pointer to member function. Additionally, the check no longer emits
   a diagnostic when a variable that is not type-dependent is an operand of a
-  type-dependent binary operator.
+  type-dependent binary operator. Improved performance of the check through
+  optimizations.
 
 - Improved :doc:`misc-include-cleaner
   <clang-tidy/checks/misc/include-cleaner>` check by adding option
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 624a643cc60e4ba..760c867ef79f74d 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -15,6 +15,76 @@
 namespace clang {
 using namespace ast_matchers;
 
+static bool canResolveToExprImpl(const Expr *Node, const Expr *Target) {
+
+  const auto IgnoreDerivedToBase = [](const Expr *E, auto Matcher) {
+    if (Matcher(E))
+      return true;
+    if (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
+      if ((Cast->getCastKind() == CK_DerivedToBase ||
+           Cast->getCastKind() == CK_UncheckedDerivedToBase) &&
+          Matcher(Cast->getSubExpr()))
+        return true;
+    }
+    return false;
+  };
+
+  const auto EvalCommaExpr = [](const Expr *E, auto Matcher) {
+    const Expr *Result = E;
+    while (const auto *BOComma =
+               dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
+      if (!BOComma->isCommaOp())
+        break;
+      Result = BOComma->getRHS();
+    }
+
+    return Result != E && Matcher(Result);
+  };
+
+  // The 'ConditionalOperatorM' matches on `<anything> ? <expr> : <expr>`.
+  // This matching must be recursive because `<expr>` can be anything resolving
+  // to the `InnerMatcher`, for example another conditional operator.
+  // The edge-case `BaseClass &b = <cond> ? DerivedVar1 : DerivedVar2;`
+  // is handled, too. The implicit cast happens outside of the conditional.
+  // This is matched by `IgnoreDerivedToBase(canResolveToExpr(InnerMatcher))`
+  // below.
+  const auto ConditionalOperatorM = [Target](const Expr *E, auto Matcher) {
+    if (const auto *OP = dyn_cast<ConditionalOperator>(E)) {
+      if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
+        if (canResolveToExprImpl(TE, Target))
+          return true;
+      if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
+        if (canResolveToExprImpl(FE, Target))
+          return true;
+    }
+    return false;
+  };
+
+  const auto ElvisOperator = [Target](const Expr *E, auto Matcher) {
+    if (const auto *OP = dyn_cast<BinaryConditionalOperator>(E)) {
+      if (const auto *TE = OP->getTrueExpr()->IgnoreParens())
+        if (canResolveToExprImpl(TE, Target))
+          return true;
+      if (const auto *FE = OP->getFalseExpr()->IgnoreParens())
+        if (canResolveToExprImpl(FE, Target))
+          return true;
+    }
+    return false;
+  };
+
+  const auto *EP = Node->IgnoreParens();
+  return IgnoreDerivedToBase(EP,
+                             [&](const Expr *E) {
+                               return E == Target ||
+                                      ConditionalOperatorM(E, Target) ||
+                                      ElvisOperator(E, Target);
+                             }) ||
+         EvalCommaExpr(EP, [&](const Expr *E) {
+           return IgnoreDerivedToBase(
+               E->IgnoreParens(), [&](const Expr *EE) { return EE == Target; });
+         });
+}
+
 namespace {
 
 AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) {
@@ -27,56 +97,14 @@ AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
   return InnerMatcher.matches(*Range, Finder, Builder);
 }
 
-AST_MATCHER_P(Expr, maybeEvalCommaExpr, ast_matchers::internal::Matcher<Expr>,
-              InnerMatcher) {
-  const Expr *Result = &Node;
-  while (const auto *BOComma =
-             dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
-    if (!BOComma->isCommaOp())
-      break;
-    Result = BOComma->getRHS();
-  }
-  return InnerMatcher.matches(*Result, Finder, Builder);
-}
-
-AST_MATCHER_P(Stmt, canResolveToExpr, ast_matchers::internal::Matcher<Stmt>,
-              InnerMatcher) {
+AST_MATCHER_P(Stmt, canResolveToExpr, const Stmt *, Inner) {
   auto *Exp = dyn_cast<Expr>(&Node);
-  if (!Exp) {
-    return stmt().matches(Node, Finder, Builder);
-  }
-
-  auto DerivedToBase = [](const ast_matchers::internal::Matcher<Expr> &Inner) {
-    return implicitCastExpr(anyOf(hasCastKind(CK_DerivedToBase),
-                                  hasCastKind(CK_UncheckedDerivedToBase)),
-                            hasSourceExpression(Inner));
-  };
-  auto IgnoreDerivedToBase =
-      [&DerivedToBase](const ast_matchers::internal::Matcher<Expr> &Inner) {
-        return ignoringParens(expr(anyOf(Inner, DerivedToBase(Inner))));
-      };
-
-  // The 'ConditionalOperator' matches on `<anything> ? <expr> : <expr>`.
-  // This matching must be recursive because `<expr>` can be anything resolving
-  // to the `InnerMatcher`, for example another conditional operator.
-  // The edge-case `BaseClass &b = <cond> ? DerivedVar1 : DerivedVar2;`
-  // is handled, too. The implicit cast happens outside of the conditional.
-  // This is matched by `IgnoreDerivedToBase(canResolveToExpr(InnerMatcher))`
-  // below.
-  auto const ConditionalOperator = conditionalOperator(anyOf(
-      hasTrueExpression(ignoringParens(canResolveToExpr(InnerMatcher))),
-      hasFalseExpression(ignoringParens(canResolveToExpr(InnerMatcher)))));
-  auto const ElvisOperator = binaryConditionalOperator(anyOf(
-      hasTrueExpression(ignoringParens(canResolveToExpr(InnerMatcher))),
-      hasFalseExpression(ignoringParens(canResolveToExpr(InnerMatcher)))));
-
-  auto const ComplexMatcher = ignoringParens(
-      expr(anyOf(IgnoreDerivedToBase(InnerMatcher),
-                 maybeEvalCommaExpr(IgnoreDerivedToBase(InnerMatcher)),
-                 IgnoreDerivedToBase(ConditionalOperator),
-                 IgnoreDerivedToBase(ElvisOperator))));
-
-  return ComplexMatcher.matches(*Exp, Finder, Builder);
+  if (!Exp)
+    return true;
+  auto *Target = dyn_cast<Expr>(Inner);
+  if (!Target)
+    return false;
+  return canResolveToExprImpl(Exp, Target);
 }
 
 // Similar to 'hasAnyArgument', but does not work because 'InitListExpr' does
@@ -121,6 +149,12 @@ AST_MATCHER_P(GenericSelectionExpr, hasControllingExpr,
   return InnerMatcher.matches(*Node.getControllingExpr(), Finder, Builder);
 }
 
+template <typename T>
+ast_matchers::internal::Matcher<T>
+findFirst(const ast_matchers::internal::Matcher<T> &Matcher) {
+  return anyOf(Matcher, hasDescendant(Matcher));
+}
+
 const auto nonConstReferenceType = [] {
   return hasUnqualifiedDesugaredType(
       referenceType(pointee(unless(isConstQualified()))));
@@ -220,8 +254,8 @@ bool ExprMutationAnalyzer::isUnevaluated(const Stmt *Exp, const Stmt &Stm,
   return selectFirst<Stmt>(
              NodeID<Expr>::value,
              match(
-                 findAll(
-                     stmt(canResolveToExpr(equalsNode(Exp)),
+                 findFirst(
+                     stmt(canResolveToExpr(Exp),
                           anyOf(
                               // `Exp` is part of the underlying expression of
                               // decltype/typeof if it has an ancestor of
@@ -275,44 +309,41 @@ const Stmt *ExprMutationAnalyzer::findDeclPointeeMutation(
 
 const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // LHS of any assignment operators.
-  const auto AsAssignmentLhs = binaryOperator(
-      isAssignmentOperator(), hasLHS(canResolveToExpr(equalsNode(Exp))));
+  const auto AsAssignmentLhs =
+      binaryOperator(isAssignmentOperator(), hasLHS(canResolveToExpr(Exp)));
 
   // Operand of increment/decrement operators.
   const auto AsIncDecOperand =
       unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
-                    hasUnaryOperand(canResolveToExpr(equalsNode(Exp))));
+                    hasUnaryOperand(canResolveToExpr(Exp)));
 
   // Invoking non-const member function.
   // A member function is assumed to be non-const when it is unresolved.
   const auto NonConstMethod = cxxMethodDecl(unless(isConst()));
 
   const auto AsNonConstThis = expr(anyOf(
-      cxxMemberCallExpr(on(canResolveToExpr(equalsNode(Exp))),
-                        unless(isConstCallee())),
+      cxxMemberCallExpr(on(canResolveToExpr(Exp)), unless(isConstCallee())),
       cxxOperatorCallExpr(callee(NonConstMethod),
-                          hasArgument(0, canResolveToExpr(equalsNode(Exp)))),
+                          hasArgument(0, canResolveToExpr(Exp))),
       // In case of a templated type, calling overloaded operators is not
       // resolved and modelled as `binaryOperator` on a dependent type.
       // Such instances are considered a modification, because they can modify
       // in different instantiations of the template.
-      binaryOperator(
-          hasEitherOperand(ignoringImpCasts(canResolveToExpr(equalsNode(Exp)))),
-          isTypeDependent()),
+      binaryOperator(isTypeDependent(),
+                     hasEitherOperand(ignoringImpCasts(canResolveToExpr(Exp)))),
       // Within class templates and member functions the member expression might
       // not be resolved. In that case, the `callExpr` is considered to be a
       // modification.
-      callExpr(
-          callee(expr(anyOf(unresolvedMemberExpr(hasObjectExpression(
-                                canResolveToExpr(equalsNode(Exp)))),
-                            cxxDependentScopeMemberExpr(hasObjectExpression(
-                                canResolveToExpr(equalsNode(Exp)))))))),
+      callExpr(callee(expr(anyOf(
+          unresolvedMemberExpr(hasObjectExpression(canResolveToExpr(Exp))),
+          cxxDependentScopeMemberExpr(
+              hasObjectExpression(canResolveToExpr(Exp))))))),
       // Match on a call to a known method, but the call itself is type
       // dependent (e.g. `vector<T> v; v.push(T{});` in a templated function).
-      callExpr(allOf(isTypeDependent(),
-                     callee(memberExpr(hasDeclaration(NonConstMethod),
-                                       hasObjectExpression(canResolveToExpr(
-                                           equalsNode(Exp)))))))));
+      callExpr(allOf(
+          isTypeDependent(),
+          callee(memberExpr(hasDeclaration(NonConstMethod),
+                            hasObjectExpression(canResolveToExpr(Exp))))))));
 
   // Taking address of 'Exp'.
   // We're assuming 'Exp' is mutated as soon as its address is taken, though in
@@ -322,11 +353,10 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       unaryOperator(hasOperatorName("&"),
                     // A NoOp implicit cast is adding const.
                     unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
-                    hasUnaryOperand(canResolveToExpr(equalsNode(Exp))));
-  const auto AsPointerFromArrayDecay =
-      castExpr(hasCastKind(CK_ArrayToPointerDecay),
-               unless(hasParent(arraySubscriptExpr())),
-               has(canResolveToExpr(equalsNode(Exp))));
+                    hasUnaryOperand(canResolveToExpr(Exp)));
+  const auto AsPointerFromArrayDecay = castExpr(
+      hasCastKind(CK_ArrayToPointerDecay),
+      unless(hasParent(arraySubscriptExpr())), has(canResolveToExpr(Exp)));
   // Treat calling `operator->()` of move-only classes as taking address.
   // These are typically smart pointers with unique ownership so we treat
   // mutation of pointee as mutation of the smart pointer itself.
@@ -334,7 +364,7 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       hasOverloadedOperatorName("->"),
       callee(
           cxxMethodDecl(ofClass(isMoveOnly()), returns(nonConstPointerType()))),
-      argumentCountIs(1), hasArgument(0, canResolveToExpr(equalsNode(Exp))));
+      argumentCountIs(1), hasArgument(0, canResolveToExpr(Exp)));
 
   // Used as non-const-ref argument when calling a function.
   // An argument is assumed to be non-const-ref when the function is unresolved.
@@ -342,8 +372,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // findFunctionArgMutation which has additional smarts for handling forwarding
   // references.
   const auto NonConstRefParam = forEachArgumentWithParamType(
-      anyOf(canResolveToExpr(equalsNode(Exp)),
-            memberExpr(hasObjectExpression(canResolveToExpr(equalsNode(Exp))))),
+      anyOf(canResolveToExpr(Exp),
+            memberExpr(hasObjectExpression(canResolveToExpr(Exp)))),
       nonConstReferenceType());
   const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
   const auto TypeDependentCallee =
@@ -354,19 +384,17 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   const auto AsNonConstRefArg = anyOf(
       callExpr(NonConstRefParam, NotInstantiated),
       cxxConstructExpr(NonConstRefParam, NotInstantiated),
-      callExpr(TypeDependentCallee,
-               hasAnyArgument(canResolveToExpr(equalsNode(Exp)))),
-      cxxUnresolvedConstructExpr(
-          hasAnyArgument(canResolveToExpr(equalsNode(Exp)))),
+      callExpr(TypeDependentCallee, hasAnyArgument(canResolveToExpr(Exp))),
+      cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
       // Previous False Positive in the following Code:
       // `template <typename T> void f() { int i = 42; new Type<T>(i); }`
       // Where the constructor of `Type` takes its argument as reference.
       // The AST does not resolve in a `cxxConstructExpr` because it is
       // type-dependent.
-      parenListExpr(hasDescendant(expr(canResolveToExpr(equalsNode(Exp))))),
+      parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
       // If the initializer is for a reference type, there is no cast for
       // the variable. Values are cast to RValue first.
-      initListExpr(hasAnyInit(expr(canResolveToExpr(equalsNode(Exp))))));
+      initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
 
   // Captured by a lambda by reference.
   // If we're initializing a capture with 'Exp' directly then we're initializing
@@ -380,76 +408,72 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // For returning by const-ref there will be an ImplicitCastExpr <NoOp> (for
   // adding const.)
   const auto AsNonConstRefReturn =
-      returnStmt(hasReturnValue(canResolveToExpr(equalsNode(Exp))));
+      returnStmt(hasReturnValue(canResolveToExpr(Exp)));
 
   // It is used as a non-const-reference for initalizing a range-for loop.
-  const auto AsNonConstRefRangeInit = cxxForRangeStmt(
-      hasRangeInit(declRefExpr(allOf(canResolveToExpr(equalsNode(Exp)),
-                                     hasType(nonConstReferenceType())))));
+  const auto AsNonConstRefRangeInit = cxxForRangeStmt(hasRangeInit(declRefExpr(
+      allOf(canResolveToExpr(Exp), hasType(nonConstReferenceType())))));
 
   const auto Matches = match(
-      traverse(TK_AsIs,
-               findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand,
-                                  AsNonConstThis, AsAmpersandOperand,
-                                  AsPointerFromArrayDecay, AsOperatorArrowThis,
-                                  AsNonConstRefArg, AsLambdaRefCaptureInit,
-                                  AsNonConstRefReturn, AsNonConstRefRangeInit))
-                           .bind("stmt"))),
+      traverse(
+          TK_AsIs,
+          findFirst(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis,
+                               AsAmpersandOperand, AsPointerFromArrayDecay,
+                               AsOperatorArrowThis, AsNonConstRefArg,
+                               AsLambdaRefCaptureInit, AsNonConstRefReturn,
+                               AsNonConstRefRangeInit))
+                        .bind("stmt"))),
       Stm, Context);
   return selectFirst<Stmt>("stmt", Matches);
 }
 
 const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) {
   // Check whether any member of 'Exp' is mutated.
-  const auto MemberExprs =
-      match(findAll(expr(anyOf(memberExpr(hasObjectExpression(
-                                   canResolveToExpr(equalsNode(Exp)))),
-                               cxxDependentScopeMemberExpr(hasObjectExpression(
-                                   canResolveToExpr(equalsNode(Exp)))),
-                               binaryOperator(hasOperatorName(".*"),
-                                              hasLHS(equalsNode(Exp)))))
-                        .bind(NodeID<Expr>::value)),
-            Stm, Context);
+  const auto MemberExprs = match(
+      findAll(expr(anyOf(memberExpr(hasObjectExpression(canResolveToExpr(Exp))),
+                         cxxDependentScopeMemberExpr(
+                             hasObjectExpression(canResolveToExpr(Exp))),
+                         binaryOperator(hasOperatorName(".*"),
+                                        hasLHS(equalsNode(Exp)))))
+                  .bind(NodeID<Expr>::value)),
+      Stm, Context);
   return findExprMutation(MemberExprs);
 }
 
 const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) {
   // Check whether any element of an array is mutated.
-  const auto SubscriptExprs =
-      match(findAll(arraySubscriptExpr(
-                        anyOf(hasBase(canResolveToExpr(equalsNode(Exp))),
-                              hasBase(implicitCastExpr(
-                                  allOf(hasCastKind(CK_ArrayToPointerDecay),
-                                        hasSourceExpression(canResolveToExpr(
-                                            equalsNode(Exp))))))))
-                        .bind(NodeID<Expr>::value)),
-            Stm, Context);
+  const auto SubscriptExprs = match(
+      findAll(arraySubscriptExpr(
+                  anyOf(hasBase(canResolveToExpr(Exp)),
+                        hasBase(implicitCastExpr(allOf(
+                            hasCastKind(CK_ArrayToPointerDecay),
+                            hasSourceExpression(canResolveToExpr(Exp)))))))
+                  .bind(NodeID<Expr>::value)),
+      Stm, Context);
   return findExprMutation(SubscriptExprs);
 }
 
 const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
   // If the 'Exp' is explicitly casted to a non-const reference type the
   // 'Exp' is considered to be modified.
-  const auto ExplicitCast = match(
-      findAll(
-          stmt(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))),
-                        explicitCastExpr(
-                            hasDestinationType(nonConstReferenceType()))))
-              .bind("stmt")),
-      Stm, Context);
+  const auto ExplicitCast =
+      match(findFirst(stmt(castExpr(hasSourceExpression(canResolveToExpr(Exp)),
+                                    explicitCastExpr(hasDestinationType(
+                                        nonConstReferenceType()))))
+                          .bind("stmt")),
+            Stm, Context);
 
   if (const auto *CastStmt = selectFirst<Stmt>("stmt", ExplicitCast))
     return CastStmt;
 
   // If 'Exp' is casted to any non-const reference type, check the castExpr.
   const auto Casts = match(
-      findAll(
-          expr(castExpr(hasSourceExpression(canResolveToExpr(equalsNode(Exp))),
-                        anyOf(explicitCastExpr(
-                                  hasDestinationType(nonConstReferenceType())),
-                              implicitCastExpr(hasImplicitDestinationType(
-                                  nonConstReferenceType())))))
-              .bind(NodeID<Expr>::value)),
+      findAll(expr(castExpr(hasSourceExpression(canResolveToExpr(Exp)),
+                            anyOf(explicitCastExpr(hasDestinationType(
+                                      nonConstReferenceType())),
+                                  implicitCastExpr(hasImplicitDestinationType(
+                                      nonConstReferenceType())))))
+                  .bind(NodeID<Expr>::value)),
       Stm, Context)...
[truncated]

clang/lib/Analysis/ExprMutationAnalyzer.cpp Outdated Show resolved Hide resolved
clang/lib/Analysis/ExprMutationAnalyzer.cpp Outdated Show resolved Hide resolved
clang/lib/Analysis/ExprMutationAnalyzer.cpp Outdated Show resolved Hide resolved
Replaced certain AST matchers in ExprMutationAnalyzer with a more direct
utilization of AST classes. The primary bottleneck was identified in the
canResolveToExpr AST matcher. Since this matcher was employed multiple
times and used recursively, each invocation led to the constant creation
and destruction of other matchers within it. Additionally, the continual
comparison of DynTypedNode resulted in significant performance degradation.

The optimization was tested on the TargetLowering.cpp file. Originally,
the check took 156 seconds on that file, but after implementing this
enhancement, it now takes approximately 40 seconds, making it nearly four
times faster.

Despite this improvement, there are still numerous issues in this file.
To further reduce the computational cost of this class, it is advisable to
consider removing the remaining matchers and exploring alternatives such as
leveraging RecursiveASTVisitor and increasing the direct use of AST classes.
@PiotrZSL PiotrZSL force-pushed the 71786-clang-tidy-misc-const-correctness-is-compared-to-other-checks-very-slow-1617 branch from 4d7cb8b to 5cd572e Compare January 5, 2024 19:23
Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PiotrZSL PiotrZSL merged commit 6eb372e into llvm:main Jan 9, 2024
5 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Replaced certain AST matchers in ExprMutationAnalyzer with a more direct
utilization of AST classes. The primary bottleneck was identified in the
canResolveToExpr AST matcher. Since this matcher was employed multiple
times and used recursively, each invocation led to the constant creation
and destruction of other matchers within it. Additionally, the continual
comparison of DynTypedNode resulted in significant performance
degradation.

The optimization was tested on the TargetLowering.cpp file. Originally,
the check took 156 seconds on that file, but after implementing this
enhancement, it now takes approximately 40 seconds, making it nearly
four times faster.

Despite this improvement, there are still numerous issues in this file.
To further reduce the computational cost of this class, it is advisable
to consider removing the remaining matchers and exploring alternatives
such as leveraging RecursiveASTVisitor and increasing the direct use of
AST classes.

Closes llvm#71786
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang Clang issues not falling into any other category clang-tidy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] misc-const-correctness is compared to other checks very slow (16/17)
3 participants