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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:541
     SimplifiedValues[&I] = C;
+    if (Lookup)
+      accumulateSavings();
----------------
eraman wrote:
> 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.
Can you add a brief comment about this?


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


================
Comment at: lib/Analysis/InlineCost.cpp:599
+  // callsite's frequency is below MinBFForSpeedupBonus.
+  if ((PSI && PSI->isHotCallSite(CS, CallerBFI)) ||
+      CallSiteFreq / EntryFreq >= MinBFForSpeedupBonus)
----------------
Better filter cold callsite out too.


================
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) {
----------------
what do the savings come from in this case? just the call overhead?


https://reviews.llvm.org/D30062