[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 a comment.

Thanks for the comments.



================
Comment at: lib/Analysis/InlineCost.cpp:89
+/// only when the callsite is relatively hot compared to caller's entry.
+static cl::opt<unsigned> MinBFForSpeedupBonus(
+    "min-freq-for-speedup-bonus", cl::Hidden, cl::init(8), cl::ZeroOrMore,
----------------
davidxl wrote:
> Does this mean globally hot callsites may not get the speed up bonus if their relative frequency to caller entry is smaller than 8? while a less hot callsite may get both hot callsite bump and big speedup bump?
That is possible.  The easy fix is to check if the callsite is hot or if it's relative frequency exceeds this threshold.  I have made this change.


================
Comment at: lib/Analysis/InlineCost.cpp:541
     SimplifiedValues[&I] = C;
+    if (Lookup)
+      accumulateSavings();
----------------
davidxl wrote:
> why does this need to be guarded here?
We want to give a bonus if the speedup only happens due to inlining. In most cases, if Evaluate returns a constant, then we would have looked up the SimplifiedValues. But it is also possible that the IR actually has an instruction with two constant operands which were not cleaned by earlier simplification pass and we don't want to consider it an inlining benefit (since a later simplification pass will simplify it). I don't think it is very likely in a reasonable pass pipeline, but explicitly having this guard makes the intention cleaner.


================
Comment at: lib/Analysis/InlineCost.cpp:989
+    // FIXME: Increase the savings associated with simplifying a callsite.
+    if (SVLookup)
+      accumulateSavings(InlineConstants::InstrCost +
----------------
davidxl wrote:
> Why is the guard needed here?
Same explanation as above.


https://reviews.llvm.org/D30062