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

[Serguei Katkov via Phabricator via llvm-commits]

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.


https://reviews.llvm.org/D30633