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

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 5 inline comments as done.
eraman added inline comments.


================
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>
----------------
chandlerc wrote:
> 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...
I've tried to expand this a bit. PTAL. 

Regarding the threshold being low:  When I tuned it originally, it was before r286814 which added -30 to the cost for every inlining. At least to account that this needs to be revised up. In any case, I'll collect more numbers on the size/performance tradeoff and follow up.


================
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"));
+
----------------
chandlerc wrote:
> 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...
It is relative. I've updated the variable name, flag name and the description to make this clear.


================
Comment at: lib/Analysis/InlineCost.cpp:532
+    bool SVLookup = false;
+    for (User::op_iterator I = GEP.idx_begin(), E = GEP.idx_end(); I != E;
+         ++I) {
----------------
chandlerc wrote:
> Prazek wrote:
> > chandlerc wrote:
> > > Prazek wrote:
> > > > range based for loop? There should be something like .operands()
> > > I looked and there isn't really. I can add one though.
> > That would be great, thanks
> Huh, I must have missed it. Danny added an 'indices' method back in 2015. =D
I see indices() in some other classes but not in GetElementPtrInst


================
Comment at: test/Transforms/Inline/speedup-analysis.ll:3
+; Test that a callee that does not fit within the threshold gets inlined
+; because of the estimated speedup heuristic.
+define i32 @caller(i32 %n) {
----------------
davidxl wrote:
> what do the savings come from in this case? just the call overhead?
Yes, the savings come from eliminating the call.


https://reviews.llvm.org/D30062