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

[Hans Wennborg via llvm-commits]

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?


Cheers,
Hans