[clang] Avoid evaluating the BitWidth expression over and over again #66203
Conversation
It's usually a ConstantExpr with a saved constant integer anyway, so we can just return that.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang ChangesIt's usually a ConstantExpr with a saved constant integer anyway, so we can just return that. -- Full diff: https://github.com//pull/66203.diff1 Files Affected:
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 6109829cf20a678..0e964b7f1372227 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4471,6 +4471,14 @@ void FieldDecl::setLazyInClassInitializer(LazyDeclStmtPtr NewInit) {
unsigned FieldDecl::getBitWidthValue(const ASTContext &Ctx) const {
assert(isBitField() && "not a bitfield");
+
+ // Try to avoid evaluating the expression in the overwhelmingly
+ // common case that we have a ConstantExpr.
+ if (const auto *CE = dyn_cast<ConstantExpr>(getBitWidth())) {
+ assert(CE->hasAPValueResult());
+ return CE->getResultAsAPSInt().getZExtValue();
+ }
+
return getBitWidth()->EvaluateKnownConstInt(Ctx).getZExtValue();
}
|
|
Hum... should we put that in |
See https://reviews.llvm.org/D155548 - I can't really support any of this with performance numbers (from my testing, I can't even support that we short-circuit |
There was a problem hiding this comment.
I knew there was a deja-vu here :)
I'd rather we do the general solution then. Maybe we should pursue the discussion on phab. I can't imagine the general solution is going to ever be noticeably worse.
I have the possibility to push a branch here http://llvm-compile-time-tracker.com/
@AaronBallman
|
@cor3ntin Looking at this again... while I'd also like to see the general solution, I'd probably apply this anyway since it makes it clearer that this function can actually be fast. Its name is terrible for what it does. It sounds like it's a simply getter but in reality it might be pretty costly to call. This patch would mitigate this somewhat and make the performance expectations clearer when reading the code of this function. |
|
Ping |
|
Ping |
|
Ping |
The bounds of a c++ array is a _constant-expression_. And in C++ it is also a constant expression. But we also support VLAs, ie arrays with non-constant bounds. We need to take care to handle the case of a consteval function (which are specified to be only immediately called in non-constant contexts) that appear in arrays bounds. This introduces `Sema::isAlwayConstantEvaluatedContext`, and a flag in ExpressionEvaluationContextRecord, such that immediate functions in array bounds are always immediately invoked. Sema had both `isConstantEvaluatedContext` and `isConstantEvaluated`, so I took the opportunity to cleanup that. The change in `TimeProfilerTest.cpp` is an unfortunate manifestation of the problem that llvm#66203 seeks to address. Fixes llvm#65520
The bounds of a c++ array is a _constant-expression_. And in C++ it is also a constant expression. But we also support VLAs, ie arrays with non-constant bounds. We need to take care to handle the case of a consteval function (which are specified to be only immediately called in non-constant contexts) that appear in arrays bounds. This introduces `Sema::isAlwayConstantEvaluatedContext`, and a flag in ExpressionEvaluationContextRecord, such that immediate functions in array bounds are always immediately invoked. Sema had both `isConstantEvaluatedContext` and `isConstantEvaluated`, so I took the opportunity to cleanup that. The change in `TimeProfilerTest.cpp` is an unfortunate manifestation of the problem that #66203 seeks to address. Fixes #65520
Once we have the general solution, this code would be redundant, wouldn't it? (I also would prefer the more general solution if reasonable -- playing whack-a-mole on constant evaluation is going to make for maintenance problems and this is an issue that plagues other parts of the compiler as well.) |
|
Well, this is not going to make a noticeable difference in runtime. https://reviews.llvm.org/D155548 didn't land because there are no measurements to make where this makes a measurable difference. As for my earlier comment, it would also make sense to rename that function to |
Those changes didn't land because no measurements were attempted. Putting up a branch at https://llvm-compile-time-tracker.com/ would help get those measurements to at least start to see if there's benefit or harm from the changes.
Caching the computed value would make sense, but that's sort of the goal of D155548, right? That's a generalized caching mechanism that should mean when we compute it to diagnose 0 size, we never need to re-compute it again. |
I did measure it, but only by myself, I can ask Nikita do use the compile-time tracker as well.
Well yes, we would never compute it again, but we still go through quite a few function calls to figure out that we've already computed it before. For a function called I thought the bitwidth expression will always be a |
Ah, I didn't realize you had measured on your own. But yeah, having some concrete numbers to play with would help.
I'm hopeful the optimizer ends up inlining at least some of those calls, but otherwise, yeah you're right.
Per spec, it needs to be a constant expression. In terms of our implementation, I would also assume it would either be |
I'd like to see rough numbers too but I'm really hoping we can progress https://reviews.llvm.org/D155548. |
|
^ Removing integer and boolean expressions seems to have some negative impact though. |
|
I've merged https://reviews.llvm.org/D155548, so closing this. |
It's usually a ConstantExpr with a saved constant integer anyway, so we can just return that.