[PATCH] D17584: Cleanup inline cost analysis code

[Xinliang David Li via llvm-commits]

On Fri, Feb 26, 2016 at 5:04 PM, Easwaran Raman <eraman at google.com> wrote:

> eraman added inline comments.
>
> ================
> 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())
> ----------------
> 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.
>
> ================
> Comment at: lib/Analysis/InlineCost.cpp:1328
> @@ -1327,3 @@
> -    if (!analyzeBlock(BB, EphValues)) {
> -      if (IsRecursiveCall || ExposesReturnsTwice || HasDynamicAlloca ||
> -          HasIndirectBr || HasFrameEscape)
> ----------------
> davidxl wrote:
> > Why are these checks removed? Are they redundant?  They do look like to
> be in  the wrong place. Can you find out the original intention of the
> code? Is there a regression test somewhere?
> I don't know why they were in the first place. If analyzeBlock returns
> false and it reaches the break stmt, it only means Cost > Threshold and we
> should be bailing out. There is no reason to break out of loop and return
> false at the end.
>

Can you do a svn blame to find more details ?

David

>
>
> http://reviews.llvm.org/D17584
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160226/fb3a17c9/attachment.html>