Skip to content

Conversation

@hazzlim
Copy link
Contributor

@hazzlim hazzlim commented Oct 17, 2024

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.

@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-function-specialization

Author: Hari Limaye (hazzlim)

Changes

Always 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:

  • (modified) llvm/lib/Transforms/IPO/FunctionSpecialization.cpp (+10-5)
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

@kiranchandramohan
Copy link
Contributor

MinFunctionSize to 400

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.
@hazzlim
Copy link
Contributor Author

hazzlim commented Oct 17, 2024

Reduces the O3 (non-LTO) compile-time of CTMark, measured by the metric of instruction count, when SpecializeLiteralConstant is enabled.

Benchmark        |  Specs  | Change  |
-----------------+---------+---------+
ClamAV           | 2 -> 2  | -0.01%  |
7zip             | 0 -> 0  |  0.00%  |
kimwitu++        | 0 -> 0  | -0.01%  |
tramp3d-v4       | 0 -> 0  |  0.00%  |
sqlite3          | 0 -> 0  |  0.00%  |
mafft            | 0 -> 0  |  0.00%  |
SPASS            | 1 -> 0  | -0.43%  |
lencod           | 3 -> 0  | -0.39%  |
consumer-typeset | 0 -> 0  | -0.02%  |
Bullet           | 0 -> 0  |  0.00%  |
-----------------+---------+---------+
GEOMEAN          |         | -0.09%  |

It has no impact on Release-LTOg builds (either with or without SpecializeLiteralConstant), as no specializations are performed.

This change is intended to allow us to enable SpecializeLiteralConstant by default, without impacting compile-time.

Copy link
Collaborator

@momchil-velikov momchil-velikov left a 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 =
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - done :)

@hazzlim hazzlim requested a review from david-arm October 17, 2024 14:14
@labrinea
Copy link
Collaborator

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.

@hazzlim
Copy link
Contributor Author

hazzlim commented Oct 17, 2024

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 :)

@labrinea
Copy link
Collaborator

Thanks for the contribution!

@hazzlim hazzlim merged commit 0d1a91e into llvm:main Oct 18, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants