[PATCH] D155548: [clang][ExprConst] Short-circuit ConstantExpr evaluation

Mon Oct 9 09:46:23 PDT 2023

aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D155548#4653363 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155548