[PATCH] D17584: Cleanup inline cost analysis code

[Easwaran Raman via llvm-commits]

It was introduced in r153812 by Chandler and AFAICT, even then the break
from the loop would eventually result in returning false.

On Fri, Feb 26, 2016 at 5:05 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> 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/3be7e423/attachment.html>