[clang][ExprConst] Short-circuit ConstantExpr evaluation

[tbaeder]

Summary

I think this makes sense to do, given that the ConstantExpr... is constant and can even already have the result attached.

[cor3ntin]

I think this does make sense. Have you notice an impact on performance?
I wonder if there are places where we should cache the result of evaluating a ConstantExpr, like in ExprEvaluatorBase::VisitConstantExpr
How often do we construct a ConstantExpr without an initial computed value?

[tbaeder]

In D155548#4509056, @cor3ntin wrote:

I think this does make sense. Have you notice an impact on performance?

No, but my local builds are debug builds and I have sanitizers enabled.
When compiling the sqlite amalgamation, I see 1590 ConstantExprs, 421 of which have their value already set.

I wonder if there are places where we should cache the result of evaluating a ConstantExpr, like in ExprEvaluatorBase::VisitConstantExpr

I thought this happens elsewhere higher up the stack. E.g., in Sema::VerifyIntegerConstantExpression,

How often do we construct a ConstantExpr without an initial computed value?

From a quick check:

clang/lib/Sema/SemaExpr.cpp:7735:25
clang/lib/Sema/SemaExpr.cpp:17937:20
clang/lib/Sema/SemaExpr.cpp:18244:23
clang/lib/Sema/SemaExpr.cpp:20165:12

(That's the places using the ::Create version without the value argument, I haven't checked ::CreateEmpty).

[tbaeder]

Ping

[aaron.ballman]

I think the changes make sense and generally look good, but because this is intended as a performance improvement, I'd like some measurements to prove it actually does improve performance. That then gives us something good to talk about in a release note as well.

[cor3ntin]

@aaron.ballman Given the recent discussion we had on both github (https://github.com/llvm/llvm-project/pull/66203) and privately, I'm going to accept that.
We only create ConstantExpr for ICE and immediate invocations, so it would have a much less limited and impact that first envisioned (we should fix that, cf https://github.com/llvm/llvm-project/issues/61425), and so we don't have good benchmarks were this patch makes things better,
but it also doesn't make things worse and in helps in limited cases.

We should probably investigate as a follow up whether we should use ConstantExpr in more places

[aaron.ballman]

In D155548#4653363, @cor3ntin wrote:

@aaron.ballman Given the recent discussion we had on both github (https://github.com/llvm/llvm-project/pull/66203) and privately, I'm going to accept that.
We only create ConstantExpr for ICE and immediate invocations, so it would have a much less limited and impact that first envisioned (we should fix that, cf https://github.com/llvm/llvm-project/issues/61425), and so we don't have good benchmarks were this patch makes things better,
but it also doesn't make things worse and in helps in limited cases.

We should probably investigate as a follow up whether we should use ConstantExpr in more places

I'm very skeptical of performance improvements where we can't actually measure that performance was improved. However, I actually agree with @cor3ntin in this case -- we're not using ConstantExpr in places where I would have expected to see it used -- instead we do a dance like we do here with IntegerLiteral and CXXBoolLiteralExpr, so this would currently require consteval-heavy code to see what kind of performance improvements there are, and there's not a lot of extant code for us to test against.

I'm accepting with some reluctance given the lack of demonstration that this is an improvement; please keep an eye out for new issues filed against Clang 18 for performance regressions. I don't expect this to cause any issues, but best to keep it in mind given the testing done showing increases in wall clock time as well as decreases. Hopefully follow-up work will improve caching by using ConstantExpr in more places.

clang/lib/AST/ExprConstant.cpp

static bool FastEvaluateAsRValue(const Expr *Exp, Expr::EvalResult &Result,
   }
 
   if (const auto *L = dyn_cast<CXXBoolLiteralExpr>(Exp)) {
     Result.Val = APValue(APSInt(APInt(1, L->getValue())));
     IsConst = true;
     return true;
   }
 
+  if (const auto *CE = dyn_cast<ConstantExpr>(Exp)) {
+    if (CE->hasAPValueResult()) {
+      Result.Val = CE->getAPValueResult();
+      IsConst = true;
+      return true;
+    }
+
+    // The SubExpr is usually just an IntegerLiteral.
+    return FastEvaluateAsRValue(CE->getSubExpr(), Result, Ctx, IsConst);
+  }
+
   // This case should be rare, but we need to check it before we check on
   // the type below.
   if (Exp->getType().isNull()) {
     IsConst = false;
     return true;
   }
 
   return false;