[FnSpecialization] Only accept codesize savings if strictly greater than the minimum amount

[bababuck]

When --funcspec-min-function-size is set to a small value, any small function (provided passed a constant) will be scored favorably in terms of code size savings due to the rounding in integer division in the following statement:

auto IsProfitable = [&]() -> bool {
  ...
  if (CodeSizeSavings < MinCodeSizeSavings * FuncSize / 100)
    return false;
  ...
};

When MinCodeSizeSavings is set by default to 20, so if FuncSize < 5 all allowed by the knob, then MinCodeSizeSavings * FuncSize / 100 evaluates to 0. As a result, a CodeSizeSavings of 0 will pass this check and be possibly considered profitable.

I don't think this is generally an issue in the wild since --funcspec-min-function-size is not usually set to small values (it defaults to 500), but might be on inline cases.

Prior to this change, unprofitable_spec specializes (with --funcspec-min-function-size=0) despite not having any savings.

define i32 @main(i32 %y) {
  %spec = call i32 @unprofitable_spec(i32 1, i32 %y)
  ret i32 %spec
}

define i32 @unprofitable_spec(i32 %x, i32 %y) {
  %add = add i32 %x, %y
  ret i32 %add
}

[llvmbot]

@llvm/pr-subscribers-function-specialization

@llvm/pr-subscribers-llvm-transforms

Author: Ryan Buchner (bababuck)

Changes

When --funcspec-min-function-size is set to a small value, any small function (provided passed a constant) will be scored favorably in terms of code size savings due to the rounding in integer division in the following statement:

auto IsProfitable = [&]() -> bool {
  ...
  if (CodeSizeSavings < MinCodeSizeSavings * FuncSize / 100)
    return false;
  ...
};

When MinCodeSizeSavings is set by default to 20, so if FuncSize < 5 all allowed by the knob, then MinCodeSizeSavings * FuncSize / 100 evaluates to 0. As a result, a CodeSizeSavings of 0 will pass this check and be possibly considered profitable.

I don't think this is generally an issue in the wild since --funcspec-min-function-size is not usually set to small values (it defaults to 500), but might be on inline cases.

Prior to this change, unprofitable_spec specializes (with --funcspec-min-function-size=0) despite not having any savings.

define i32 @<!-- -->main(i32 %y) {
  %spec = call i32 @<!-- -->unprofitable_spec(i32 1, i32 %y)
  ret i32 %spec
}

define i32 @<!-- -->unprofitable_spec(i32 %x, i32 %y) {
  %add = add i32 %x, %y
  ret i32 %add
}

---

Full diff: https://github.com/llvm/llvm-project/pull/164867.diff

5 Files Affected:

• (modified) llvm/lib/Transforms/IPO/FunctionSpecialization.cpp (+1-1)
• (modified) llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll (+2-1)
• (modified) llvm/test/Transforms/FunctionSpecialization/maxgrowth.ll (+1-1)
• (added) llvm/test/Transforms/FunctionSpecialization/min-codesize-savings.ll (+28)
• (modified) llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll (+2-1)

diff --git a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
index 150a2dc5d48e2..6d4b2fb7e0065 100644
--- a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
@@ -995,7 +995,7 @@ bool FunctionSpecializer::findSpecializations(Function *F, unsigned FuncSize,
                    << (CodeSizeSavings * 100 / FuncSize) << "%)}\n");
 
         // Minimum codesize savings.
-        if (CodeSizeSavings < MinCodeSizeSavings * FuncSize / 100)
+        if (CodeSizeSavings <= MinCodeSizeSavings * FuncSize / 100)
           return false;
 
         // Lazily compute the Latency, to avoid unnecessarily computing BFI.
diff --git a/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll b/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll
index 134a79d349035..45e41bd1bdce9 100644
--- a/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll
+++ b/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt -passes=ipsccp  --funcspec-min-function-size=1 -S < %s | FileCheck %s
+; RUN: opt -passes=ipsccp  --funcspec-min-function-size=1 \
+; RUN: -funcspec-min-codesize-savings=1 -S < %s | FileCheck %s
 
 @gv = internal global ptr null
 
diff --git a/llvm/test/Transforms/FunctionSpecialization/maxgrowth.ll b/llvm/test/Transforms/FunctionSpecialization/maxgrowth.ll
index 82d1f7ae4a6e1..c07948489dd5e 100644
--- a/llvm/test/Transforms/FunctionSpecialization/maxgrowth.ll
+++ b/llvm/test/Transforms/FunctionSpecialization/maxgrowth.ll
@@ -1,7 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --include-generated-funcs --version 5
 ; RUN: opt -passes="ipsccp<func-spec>" -funcspec-min-function-size=1       \
 ; RUN:                                 -funcspec-for-literal-constant=true \
-; RUN:                                 -funcspec-min-codesize-savings=50   \
+; RUN:                                 -funcspec-min-codesize-savings=1    \
 ; RUN:                                 -funcspec-min-latency-savings=50    \
 ; RUN:                                 -funcspec-max-codesize-growth=1     \
 ; RUN:                                 -S < %s | FileCheck %s
diff --git a/llvm/test/Transforms/FunctionSpecialization/min-codesize-savings.ll b/llvm/test/Transforms/FunctionSpecialization/min-codesize-savings.ll
new file mode 100644
index 0000000000000..4547e0e009852
--- /dev/null
+++ b/llvm/test/Transforms/FunctionSpecialization/min-codesize-savings.ll
@@ -0,0 +1,28 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -passes=ipsccp  --funcspec-min-function-size=0 -S < %s | FileCheck %s
+
+; Call to unprofitable_spec should not specialize since no folding can occur for the add
+; instruction as only 1 of the values is know. The score for this specialization will be 0.
+define i32 @main(i32 %y) {
+; CHECK-LABEL: define i32 @main(
+; CHECK-SAME: i32 [[Y:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[SPEC:%.*]] = call i32 @unprofitable_spec(i32 1, i32 [[Y]])
+; CHECK-NEXT:    ret i32 [[SPEC]]
+;
+entry:
+  %spec = call i32 @unprofitable_spec(i32 1, i32 %y)
+  ret i32 %spec
+}
+
+define i32 @unprofitable_spec(i32 %x, i32 %y) {
+; CHECK-LABEL: define i32 @unprofitable_spec(
+; CHECK-SAME: i32 [[X:%.*]], i32 [[Y:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[ADD:%.*]] = add i32 [[X]], [[Y]]
+; CHECK-NEXT:    ret i32 [[ADD]]
+;
+entry:
+  %add = add i32 %x, %y
+  ret i32 %add
+}
diff --git a/llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll b/llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll
index fc17387dec94d..409e7b870355d 100644
--- a/llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll
+++ b/llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll
@@ -1,7 +1,8 @@
 ; REQUIRES: asserts
 ; RUN: opt -passes="ipsccp<func-spec>,inline,instcombine,simplifycfg" -S \
 ; RUN:     -funcspec-min-function-size=23 -funcspec-max-iters=100 \
-; RUN:     -debug-only=function-specialization < %s 2>&1 | FileCheck %s
+; RUN:     -debug-only=function-specialization -funcspec-min-codesize-savings=1 \
+; RUN:     < %s 2>&1 | FileCheck %s
 
 ; Make sure the number of specializations created are not
 ; linear to the number of iterations (funcspec-max-iters).

[mgudim]

LGTM

[labrinea]

When MinCodeSizeSavings is set by default to 20, so if FuncSize < 5 all allowed by the knob, then MinCodeSizeSavings * FuncSize / 100 evaluates to 0. As a result, a CodeSizeSavings of 0 will pass this check and be possibly considered profitable.

20% is the bare minimum threshold (see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp#L72), you are now making it higher. With your change given MinCodeSizeSavings=25 and FuncSize=4 a specialization is not profitable, since MinCodeSizeSavings must be at least 50%.

Prior to this change, unprofitable_spec specializes (with --funcspec-min-function-size=0) despite not having any savings.

Again I am not seeing the issue here. 20% of zero is zero, --funcspec-min-function-size=0 suggests the user wants everything to be considered profitable.

[bababuck]

Sorry for the slow response, out on trip. Thanks for taking the time to respond.

When MinCodeSizeSavings is set by default to 20, so if FuncSize < 5 all allowed by the knob, then MinCodeSizeSavings * FuncSize / 100 evaluates to 0. As a result, a CodeSizeSavings of 0 will pass this check and be possibly considered profitable.

20% is the bare minimum threshold (see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp#L72), you are now making it higher. With your change given MinCodeSizeSavings=25 and FuncSize=4 a specialization is not profitable, since MinCodeSizeSavings must be at least 50%.

Would something like the following be preferable? It would leave the behavior the same in the case you mention.

auto IsProfitable = [&]() -> bool {
  ...
-  if (CodeSizeSavings < MinCodeSizeSavings * FuncSize / 100)
+  if ((CodeSizeSavings < MinCodeSizeSavings * FuncSize / 100) && CodeSizeSavings > 0)
     return false;
  ...
};

Prior to this change, unprofitable_spec specializes (with --funcspec-min-function-size=0) despite not having any savings.

Again I am not seeing the issue here. 20% of zero is zero, --funcspec-min-function-size=0 suggests the user wants everything to be considered profitable.

My understanding is that --funcspec-min-function-size=0 would imply that the user would want all functions to be considered regardless of size, and that --funcspec-min-codesize-savings=0 would imply that everything should be considered profitable with respect to code size.