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