[PATCH] D131287: Fix branch weight in FoldCondBranchOnValueKnownInPredecessor pass in SimplifyCFG

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-cond-bi-fix-weight.ll:58
+;; After the patch, the branch weights from entry are updated as 1:19, then the
+;; probability of hitting foo is 29/200(14.5%), which is much closer to original one
+define void @branchinst_noweight(i8 %a0, i8 %a1, i8 %a2) {
----------------
LukeZhuang wrote:
> davidxl wrote:
> > By looking at example only, it looks like the right way is to compute the branch weights for entry block using backward propagation.
> > 
> > Say entry->lor.end  has weights x, and sentry->lor.rhs has weights y.  Assuming entry (and lor.end) has weights 100.  The following system equation holds (assumes no path sensitivity, and thus the branch weights after transformation remains the same for lor.rhs), we have
> > 
> >  x+ y = 100
> >   x + y/10 = 10
> > 
> > And it gives x = 0, y = 100
> > 
> > 
> > Please note that, due to the possibility of path sensitive behavior, there is no reason to believe that probability of lor.rhs without this patch is any worse than the one with the patch.
> Hi David, thanks for the comments!
> I have a question on it. The backward propagation require us to know the block frequency info(or especially know the whole CFG), may I ask is that correct? Otherwise I think we cannot know lor.end has the same weight as entry, and if.then has 10% weights of entry.
> So I think that goes back to our previous discussion. The first issue is the transformation in SimplifyCFG does not know either BlockFrequencyInfo nor the whole CFG. The second issue is this branch weight maintenance can also happen with static code prediction itself(using __builtin_expect in my example), in which case we do not have frequency info from dynamic analysis such as PGO.
> Please correct me if I understand it incorrectly.
The branch weights are relative numbers (to compute branch probability), comparison between weights between nodes are meaningless. BFI uses BPI information to compute block frequencies. Block frequencies are relative but at function level -- comparing block frequency across functions are not meaningful. With function entry count (real profile) available, block frequency can be converted to block count which is globally (whole program) valid and comparable.

In other words,  with a single entry/exit region, you should be able to propagate branch weights (to be more precise, to propagate taken/notaken ratio, or probability, not the raw weight values). You can assume entry node with any frequency for that matter.

The key problem with back propagation is that it assumes the probability of lor.lhs does not change after the splitting. This assumption may not hold in reality.  It is equally reasonable to assume a probability at entry node and do forward propagation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131287/new/

https://reviews.llvm.org/D131287