[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)

2024-01-08 Thread Congcong Cai via cfe-commits 

https://github.com/HerrCai0907 approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/72705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)

2024-01-05 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL updated 
https://github.com/llvm/llvm-project/pull/72705

>From 1f9b443a92446ae36825152535706037ef2e87b4 Mon Sep 17 00:00:00 2001
From: Piotr Zegar 
Date: Wed, 15 Nov 2023 16:52:00 +
Subject: [PATCH 1/2] [clang-tidy] Improve performance of
 misc-const-correctness

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.
---
 clang-tools-extra/docs/ReleaseNotes.rst |   3 +-
 clang/lib/Analysis/ExprMutationAnalyzer.cpp | 350 +++-
 2 files changed, 188 insertions(+), 165 deletions(-)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..2960ef644fde4b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -358,7 +358,8 @@ Changes in existing checks
   ` 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
   ` check by adding option
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp 
b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 624a643cc60e4b..760c867ef79f74 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(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(Result->IgnoreParens())) {
+  if (!BOComma->isCommaOp())
+break;
+  Result = BOComma->getRHS();
+}
+
+return Result != E && Matcher(Result);
+  };
+
+  // The 'ConditionalOperatorM' matches on ` ?  : `.
+  // This matching must be recursive because `` can be anything resolving
+  // to the `InnerMatcher`, for example another conditional operator.
+  // The edge-case `BaseClass  =  ? 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(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(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) {

[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)

2023-12-11 Thread Congcong Cai via cfe-commits 


@@ -15,6 +15,76 @@
 namespace clang {
 using namespace ast_matchers;
 
+static bool canResolveToExprImpl(const Expr *Node, const Expr *Target) {

HerrCai0907 wrote:

This function is hard to understand because it used matcher style naming but 
logic operator style code.

https://github.com/llvm/llvm-project/pull/72705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)

2023-12-11 Thread Congcong Cai via cfe-commits 


@@ -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(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(Result->IgnoreParens())) {
+  if (!BOComma->isCommaOp())
+break;
+  Result = BOComma->getRHS();
+}
+
+return Result != E && Matcher(Result);
+  };
+
+  // The 'ConditionalOperatorM' matches on ` ?  : `.
+  // This matching must be recursive because `` can be anything resolving
+  // to the `InnerMatcher`, for example another conditional operator.
+  // The edge-case `BaseClass  =  ? 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) {

HerrCai0907 wrote:

useless arguments `Matcher`

https://github.com/llvm/llvm-project/pull/72705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Improve performance of misc-const-correctness (PR #72705)

2023-11-17 Thread via cfe-commits 

llvmbot wrote:



@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
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
   ` 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
   ` 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(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(Result->IgnoreParens())) {
+  if (!BOComma->isCommaOp())
+break;
+  Result = BOComma->getRHS();
+}
+
+return Result != E && Matcher(Result);
+  };
+
+  // The 'ConditionalOperatorM' matches on ` ?  : `.
+  // This matching must be recursive because `` can be anything resolving
+  // to the `InnerMatcher`, for example another conditional operator.
+  // The edge-case `BaseClass  =  ? 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(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(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;