[PATCH] D150649: [FuncSpec] Enable specialization of literal constants.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:22-23
 //   but that's off by default under an option.
 // - The cost-model could be further looked into (it mainly focuses on inlining
 //   benefits),
 //
----------------
nit: It is better to have a paragraph for the cost model. This is not required now. We can add one after we feel it is relatively stable.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:495-507
+    // Check the function entry frequency only once. We sink this code here to
+    // postpone BFI until we know for sure there are Specialization candidates,
+    // otherwise we are adding unnecessary overhead.
+    if (!HasCheckedEntryFreq) {
+      // Reject cold functions (for some definition of 'cold').
+      uint64_t EntryFreq = (GetBFI)(*F).getEntryFreq();
+      if (!ForceSpecialization && EntryFreq < MinEntryFreq)
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > Can't we move this out of the loop simply?
> It's explained in the comments. If we hoist this code we are eagerly asking for the BlockFrequencyAnalysis to run even if no specializations are found. I've checked and moving it regresses compilation times for benchmarks with no specializations.
Got it. Thanks : )


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:527
         Score += getSpecializationBonus(A.Formal, A.Actual, KnownConstants);
+      Score /= SpecCost;
 
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > What's the reason that we use `/` instead of `-` here?
> Because we want the Bonus to be at least `MinScore` times higher than SpecCost. The delta was too aggressive heuristic. A ratio seems more sensible.
I still don't understand it a lot. But maybe this is what heuristic model is.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150649