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

[Zhi Zhuang via Phabricator via llvm-commits]

LukeZhuang added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2998
 
+/// We want to make the new branch probability of PTI taken(from PredBB to BB)
+/// to be original one times the probability of BI taken(from BB to Dest),
----------------
paulkirth wrote:
> What is PTI? Is this a common term from somewhere? If it's the predecessor, then `Pred` or `PredInst` seems to communicate more.
I think I used this name because I found some other functions also uses it. For example, https://github.com/llvm/llvm-project/blob/49e7ef2c09facd722a29a5ad96a7f8f16e362b28/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L1390, as well as #L1096. Yeah I agree that that might be confusing if reading the definition first before its usage. I added description to it. Thanks for pointing out


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3007
+/// The value will not overflow if totW is in uint32_t range(see details below)
+static void ScaleWeights(uint64_t BITakenWeight, uint64_t BINotTakenWeight,
+                         SmallVector<uint64_t, 8> &PTIWeights,
----------------
paulkirth wrote:
> This function seems to go to a lot of trouble to avoid overflowing `uint32_t`, which is good, but it seems like things would be significantly simpler if you did the calculations with `uint64_t` and then scaled the weights the same way we do elsewhere. If that is common functionality, then it can be moved to elsewhere, like `IR/ProfDataUtils`.
> 
> Are there other reasons other reasons, concerns, or context here that I'm missing?
Currently I already did this in uint64_t. I need to add the overflow handling because in the original formula, there will be a product of three uint32_t(totW * Weights[i] * BNTW) which will overflow even in uint64_t.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3012
+      std::accumulate(PTIWeights.begin(), PTIWeights.end(), (uint64_t)0);
+  assert(PTITotalWeight <= UINT32_MAX);
+  uint64_t PTITotalTakenWeight = 0;
----------------
paulkirth wrote:
> I don't think it's correct to assert this, is it? Weights are allowed to be 32-bits, and summing them could overflow `uint32_t`, and is an expected possibility (see `ProfDataUtils.cpp`). So why is this case different?
As we previously discussed, https://github.com/llvm/llvm-project/blob/49e7ef2c09facd722a29a5ad96a7f8f16e362b28/llvm/lib/Analysis/BranchProbabilityInfo.cpp#L432 also asserts that


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3171-3172
+    if (BIHasWeights) {
+      if (!CB->getZExtValue())
+        std::swap(BITakenWeight, BINotTakenWeight);
+      FixupBranchWeight(EdgeBB, PredBBs, BITakenWeight, BINotTakenWeight);
----------------
paulkirth wrote:
> Are we inverting the branch condition? why are we swapping? I think I've missed something here...
Because in the previous line of "BIHasWeights" I assumed that the "true successor" is BB->RealDest and "false successor" is the other one when calling  extractBranchWeights. This line is used to swap them if "false successor" is actually BB->RealDest.
Sorry for the confusing, I also added comments to avoid misunderstanding


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

https://reviews.llvm.org/D131287