[PATCH] D17584: Cleanup inline cost analysis code

[Easwaran Raman via llvm-commits]

eraman marked 2 inline comments as done.
eraman added a comment.

In http://reviews.llvm.org/D17584#362398, @ahatanak wrote:

> I'm confused with the meaning of threshold here.
>
> In the loop which visits the subset of basic blocks in the function, we use this exit condition:
>
> if (Cost > Threshold)
>
> This indicates as long as Cost doesn't exceed Threshold, the function is still eligible for inlining. If the function's Cost is equal to Threshold, it can still be inlined.
>
> However, the last statement indicates that Cost has to be smaller than Threshold in order for it to be inlined:
>
> return Cost < std::max(1, Threshold);
>
> Shouldn't the loop exit when Cost >= Threshold? In that case, the minimum value of Threshold should be 1, not 0, to enable inlining zero-cost functions.


At worst, this delays early bailout. If we change it to  Cost >= Threshold, then we need to handle the special case of Cost == 0 and Threshold == 0.  I don't like this special casing of 0 cost (the Cost <= std::max(1, Threshold) check), but reverting that would require investigating the size increase seen by Hans.


================
Comment at: lib/Analysis/InlineCost.cpp:577
@@ -574,1 +576,3 @@
 
+bool CallAnalyzer::allowSizeGrowth(CallSite CS) {
+  // If the normal destination of the invoke or the parent block of the call
----------------
davidxl wrote:
> allowSizeGrow does not sound like some property suitable for callsite. How about IsInUnreachablePath(..)
I wanted a generic name to decide whether to set Threshold to 0. 

================
Comment at: lib/Analysis/InlineCost.cpp:1376
@@ -1375,3 +1375,3 @@
       // Take off the bonus we applied to the threshold.
-      Threshold -= SingleBBBonus;
+      Threshold = std::max(Threshold - SingleBBBonus, 0);
       SingleBB = false;
----------------
davidxl wrote:
> Is this one needed now?
It's not strictly needed, but I think it makes the intention clear that Threshold is bounded below by 0.


http://reviews.llvm.org/D17584