[PATCH] D30062: Estimate speedup due to inlining and use that to adjust threshold.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Some initial (minor) comments. It'd also help I think to rebase this as some of the refactorings land.



================
Comment at: lib/Analysis/InlineCost.cpp:80
+                        cl::desc("Bonus for callees showing speedup"));
+/// Minimum estimated speedup required to apply a threshold bonus.
+static cl::opt<int>
----------------
nit: Please use vertical whitespace before the comment for the second flag.

More substantive comment, what unit is this? The "desc" string only says "speedup", but it isn't clear if you mean "10% faster after inlining"? If so, that seems a surprisingly low threshold...


================
Comment at: lib/Analysis/InlineCost.cpp:90-91
+static cl::opt<unsigned> MinBFForSpeedupBonus(
+    "min-freq-for-speedup-bonus", cl::Hidden, cl::init(8), cl::ZeroOrMore,
+    cl::desc("Min relative callsite freq to apply speedup bonus"));
+
----------------
The flag seems to indicate this is the value of the frequency itself... the comment seems to indicate it is relative... Makes it hard for me to understand the value used...


================
Comment at: lib/Analysis/InlineCost.cpp:175
+  void accumulateCost(int InstructionCost);
+  void accumulateSavings(int Savings = InlineConstants::InstrCost);
   bool isGEPFree(GetElementPtrInst &GEP);
----------------
I think it'd be better to have the argument always passed and match the accumulateCost API?



https://reviews.llvm.org/D30062