[PATCH] D157123: [FuncSpec] Rework the discardment logic for unprofitable specializations.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

For the practical effects, I feel good as long as the interesting benchmarks remains optimized.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:108
+    "funcspec-min-inlining-savings", cl::init(300), cl::Hidden, cl::desc(
+    "Reject specializations whose inlining savings are less than this"
+    "much percent of the original function size"));
----------------
What is a `inlining saving`? I am not sure if this is a wide known definition.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:833
       Bonus B;
+      unsigned Score = 0;
       InstCostVisitor Visitor = getInstCostVisitorFor(F);
----------------
May this be a better name?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:851
+        // Minimum inlining savings.
+        bool HasInliningBonus = Score > MinInliningSavings * FuncSize / 100;
+        // Minimum codesize savings.
----------------
What's the meaning of the magic number `100`?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:851-857
+        bool HasInliningBonus = Score > MinInliningSavings * FuncSize / 100;
+        // Minimum codesize savings.
+        if (B.CodeSize < MinCodeSizeSavings * FuncSize / 100)
+          return HasInliningBonus;
+        // Minimum latency savings.
+        if (B.Latency < MinLatencySavings * FuncSize / 100)
+          return HasInliningBonus;
----------------
ChuanqiXu wrote:
> What's the meaning of the magic number `100`?
nit: may this be slightly more clear?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157123