[PATCH] D17584: Cleanup inline cost analysis code

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

You have three only vaguely related changes here.  In general, this should have been split up.  I'm going to request that the third piece be split and reviewed separately.  Once that's done, the first two LGTM w/noted changes as well.


================
Comment at: lib/Analysis/InlineCost.cpp:1319-1320
@@ -1305,9 +1318,4 @@
   // Note that we *must not* cache the size, this loop grows the worklist.
   for (unsigned Idx = 0; Idx != BBWorklist.size(); ++Idx) {
-    // Bail out the moment we cross the threshold. This means we'll under-count
-    // the cost, but only when undercounting doesn't matter.
-    if (Cost > Threshold)
-      break;
-
     BasicBlock *BB = BBWorklist[Idx];
     if (BB->empty())
----------------
eraman wrote:
> davidxl wrote:
> > why is the early bail out removed?
> Cost cannot be greater than Threshold in the first iteration - since there is a similar check in line 1245 above. Subsequently, if Cost exceeds Threshold, that's caught in analyzeBB which returns false and the bailout happens in that case. 
Please make this an assertion since it's a non-obvious loop invariant.

================
Comment at: lib/Analysis/InlineCost.cpp:1372
@@ -1375,3 +1371,3 @@
       // Take off the bonus we applied to the threshold.
-      Threshold -= SingleBBBonus;
+      Threshold = std::max(Threshold - SingleBBBonus, 0);
       SingleBB = false;
----------------
This should be separated into it's own change.  It isn't obvious to me that it's correct.  (Not saying it isn't, just not obvious it is.)  Separate this into it's own patch and add an assertion of the invariant you're asserting (i.e Threshold >= 0)

(Actually, you do that below.  Still separate and separately review please.)


http://reviews.llvm.org/D17584