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

[Easwaran Raman via llvm-commits]

"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.

On Fri, Nov 6, 2015 at 3:41 PM, Hans Wennborg <hans at chromium.org> wrote:

> 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?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151106/fe9a3d63/attachment.html>