[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-overflow.ll:10
+
+;; Create a test case that will definitely overflow when doing naive add/mul,
+;; by making branch_weights which used in the calculation to be UINT32_MAX.
----------------
In this case, why does the prof meta data for the switch need to change in the first place? Can the original meta data be retained?

Regarding overflow,  should the branch weights be scaled down first to avoid the problem in the first place?


================
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) {
----------------
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.


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

https://reviews.llvm.org/D131287