-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[FuncSpec] Update MinFunctionSize logic #112711
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
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-function-specialization Author: Hari Limaye (hazzlim) ChangesAlways require functions to be larger than MinFunctionSize when SpecializeLiteralConstant is enabled, and increase MinFunctionSize to 400, to prevent excessive triggering of specialisations on small functions. Full diff: https://github.com/llvm/llvm-project/pull/112711.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
index bd0a337e579e48..6d5c8b4229a417 100644
--- a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
@@ -57,9 +57,9 @@ static cl::opt<unsigned> MaxBlockPredecessors(
"considered during the estimation of dead code"));
static cl::opt<unsigned> MinFunctionSize(
- "funcspec-min-function-size", cl::init(300), cl::Hidden, cl::desc(
- "Don't specialize functions that have less than this number of "
- "instructions"));
+ "funcspec-min-function-size", cl::init(500), cl::Hidden,
+ cl::desc("Don't specialize functions that have less than this number of "
+ "instructions"));
static cl::opt<unsigned> MaxCodeSizeGrowth(
"funcspec-max-codesize-growth", cl::init(3), cl::Hidden, cl::desc(
@@ -641,12 +641,17 @@ bool FunctionSpecializer::run() {
Metrics.analyzeBasicBlock(&BB, GetTTI(F), EphValues);
}
+ // When specializing literal constants is enabled, always require functions
+ // to be larger than MinFunctionSize, to prevent excessive specialization.
+ bool RequireMinSize =
+ !ForceSpecialization &&
+ (SpecializeLiteralConstant || !F.hasFnAttribute(Attribute::NoInline));
+
// If the code metrics reveal that we shouldn't duplicate the function,
// or if the code size implies that this function is easy to get inlined,
// then we shouldn't specialize it.
if (Metrics.notDuplicatable || !Metrics.NumInsts.isValid() ||
- (!ForceSpecialization && !F.hasFnAttribute(Attribute::NoInline) &&
- Metrics.NumInsts < MinFunctionSize))
+ (RequireMinSize && Metrics.NumInsts < MinFunctionSize))
continue;
// TODO: For now only consider recursive functions when running multiple
|
Nit: It is 500 in the source. |
Always require functions to be larger than MinFunctionSize when SpecializeLiteralConstant is enabled, and increase MinFunctionSize to 500, to prevent excessive triggering of specialisations on small functions.
2028961 to
a70bd41
Compare
|
Reduces the O3 (non-LTO) compile-time of CTMark, measured by the metric of instruction count, when It has no impact on Release-LTOg builds (either with or without This change is intended to allow us to enable |
momchil-velikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| // When specializing literal constants is enabled, always require functions | ||
| // to be larger than MinFunctionSize, to prevent excessive specialization. | ||
| bool RequireMinSize = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May add "const" for a little bit of extra self-documentation. Or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - done :)
|
The test that was added in https://reviews.llvm.org/D135862 will start failing once we enable specilaziation of literal constatnts. You can fix it now or later by adding -funcspec-for-literal-constant=false on the RUN line. |
Good point - I think that it makes sense to fix things / make it explicit in this change. Done :) |
|
Thanks for the contribution! |
Always require functions to be larger than MinFunctionSize when SpecializeLiteralConstant is enabled, and increase MinFunctionSize to 500, to prevent excessive triggering of specialisations on small functions.