[PATCH] D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0

[Serguei Katkov via llvm-commits]

Thank you,
Serguei.

> -----Original Message-----
> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
> Sent: Thursday, May 18, 2017 11:25 AM
> To: reviews+D30633+public+e089212daf6c86e9 at reviews.llvm.org
> Cc: Serguei Katkov <serguei.katkov at azul.com>; chandlerc at gmail.com;
> sanjoy at playingwithpointers.com; vsk at apple.com; congh at google.com;
> junbuml at codeaurora.org; davidxl at google.com; llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D30633: [BPI] Reduce the probability of unreachable
> edge to minimal value greater than 0
> 
> 
> > On 2017-May-17, at 21:22, Serguei Katkov via Phabricator
> <reviews at reviews.llvm.org> wrote:
> >
> > skatkov added inline comments.
> >
> >
> > ================
> > Comment at: lib/Analysis/BranchProbabilityInfo.cpp:202-206
> > +  auto UnreachableProb = UR_TAKEN_PROB;  auto ReachableProb =
> > +      (BranchProbability::getOne() - UR_TAKEN_PROB *
> UnreachableEdges.size()) /
> > +      ReachableEdges.size();
> >
> > ----------------
> > dexonsmith wrote:
> >> skatkov wrote:
> >>> dexonsmith wrote:
> >>>> This doesn't look to me like it will add up to
> `BranchProbability::getOne()` in the end.  Is that a problem?
> >>> The sum of probabilities might be different than 1 and this consistent
> with all other heuristics in this file.
> >>> It seems that it was not considered as problem before.
> >>>
> >>> If you think it should be fixed I can come up with a follow-up patch fixing
> all cases. The question is actually where to add the remainder.
> >> A follow-up patch seems fine.  I suggest spreading the remainder among
> the branches, 1 each until you run out, skipping the unreachable branch.
> > Yest It is clear, the question is the following:
> > we have 3 branches, 1 unreachable and 2 reachable. The probability of
> unreachable is 1/0x80000000. Each reachable branch has
> 0x3fffffff/0x80000000. The remainder is 1. (Please note that with the current
> patch this remainder will always less then number of reachable branches). So
> we have two reachable branches and remainder is 1. Where to put it into the
> first reachable branch or second?
> >
> > In reality it might be more useful information that two reachable branches
> has the equal probability then sum of probabilities is equal to one.
> > Please also note that all other heuristics in these file do not add this
> remainder. Also please not that before this patch remainder for unreachable
> heuristic was not ignored as well.
> >
> > So if I do follow-up patch, it makes sense to fix all other cases as well.
> However I do not know the clear answer at this moment where to add this
> remainder. That is the problem.
> >
> 
> IMO, it doesn't matter which branch you put it on.  If we have some
> optimization that will strongly prefer 0x40000000/0x80000000 over
> 0x3fffffff/0x80000000 (or pessimize the latter), then we have a bug.  Being
> "exactly equal" doesn't seem like a relevant thing to check.


MachineBlockPlacement is an example of this pass. It multiplies the block frequency to branch probability to detect best exits.
It has some parameter to take into account some delta but it is set to 0 by defaut.
And I do not know how many other places does not take into account some delta.
So in general it might be dangerous.

> 
> 
> >
> > https://reviews.llvm.org/D30633
> >
> >
> >