[llvm] r238276 - [inliner] Fix the early-exit of the inline cost analysis to correctly

[Hans Wennborg via llvm-commits]

On Wed, Nov 4, 2015 at 10:55 AM, Hans Wennborg <hans at chromium.org> wrote:
> I think this is the cause of https://llvm.org/bugs/show_bug.cgi?id=24851
> Not sure if it's a big deal, but it does mean we will no longer inline
> a zero-cost noreturn function.
>
> On Tue, May 26, 2015 at 7:49 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>> Author: chandlerc
>> Date: Tue May 26 21:49:05 2015
>> New Revision: 238276
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=238276&view=rev
>> Log:
>> [inliner] Fix the early-exit of the inline cost analysis to correctly
>> model the dense vector instruction bonuses.
>>
>> Previously, this code really didn't effectively compute the density of
>> inlined vector instructions and apply the intended inliner bonus. It
>> would try to compute it repeatedly while analyzing the function and
>> didn't handle the case where future vector instructions would tip the
>> scales back towards the bonus.
>>
>> Instead, speculatively apply all possible bonuses to the threshold
>> initially. Once we *know* that a certain bonus can not be applied,
>> subtract it. This should delay early bailout enough to get much more
>> consistent results without actually causing us to analyze huge swaths of
>> code. I expect some (hopefully mild) compile time hit here, and some
>> swings in performance, but this was definitely the intended behavior of
>> these bonuses.
>>
>> This also dramatically simplifies the computation of the bonuses to not
>> interact with each other in confusing ways. The previous code didn't do
>> a good job of this and the values for bonuses may be surprising but are
>> at least now clearly written in the code.
>>
>> Finally, fix code to be in line with comments and use zero as the
>> bailout condition.
>>
>> Patch by Easwaran Raman, with some comment tweaks by me to try and
>> further clarify what is going on with this code.
>>
>> http://reviews.llvm.org/D8267
>
> [...]
>
>> @@ -1081,9 +1082,9 @@ bool CallAnalyzer::analyzeCall(CallSite
>>    Instruction *Instr = CS.getInstruction();
>>    if (InvokeInst *II = dyn_cast<InvokeInst>(Instr)) {
>>      if (isa<UnreachableInst>(II->getNormalDest()->begin()))
>> -      Threshold = 1;
>> +      Threshold = 0;
>>    } else if (isa<UnreachableInst>(++BasicBlock::iterator(Instr)))
>> -    Threshold = 1;
>> +    Threshold = 0;
>
> I think the reason these were set to 1 before is because we check
> whether Cost < Threshold, and that will now be false for Cost == 0.
>
> Also, since we speculatively added bonuses above and will subtract
> them below, we will end up with a negative Threshold.
>
> One fix would be to change the final return line to "return Cost == 0
> || Cost < Threshold". Does that sound like a reasonable fix?

Ping?