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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
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"));
----------------
ChuanqiXu wrote:
> What is a `inlining saving`? I am not sure if this is a wide known definition.
I can rename it to MinInliningBonus


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:851
+        // Minimum inlining savings.
+        bool HasInliningBonus = Score > MinInliningSavings * FuncSize / 100;
+        // Minimum codesize savings.
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > What's the meaning of the magic number `100`?
> nit: may this be slightly more clear?
Percentage. We can't do floating point decision so we multiply by MinSavings and then divide by 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;
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > ChuanqiXu wrote:
> > > What's the meaning of the magic number `100`?
> > nit: may this be slightly more clear?
> Percentage. We can't do floating point decision so we multiply by MinSavings and then divide by 100.
Okay


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157123