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

Wed Aug 24 08:35:22 PDT 2022

LukeZhuang added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1100
+/// Keep halving the weights until the sum of weights can fit in uint32_t.
+static void FitTotalWeights(MutableArrayRef<uint64_t> Weights) {
+  uint64_t Sum = std::accumulate(Weights.begin(), Weights.end(), (uint64_t)0);
----------------
paulkirth wrote:
> I’m fairly certain we don’t want to scale the weights down so that the sum of all weights fits in a uint32. 
> 
> For one there are all kinds of things that assume they can be larger. Off the top of my head, Optimization Remarks are often filtered by hotness (i.e. branch weight). But IIRC there are also transforms that only trigger when the weights exceed some threshold. Those may be better served by using a probability, but my recollection is that they do not. Though on this point someone w/ more comprehensive knowledge in this area should probably weigh in. Still, I'm skeptical that we want to do this.
> 
> I'm a bit surprised that scaling weights down like this does not affect any tests. That may be highlighting some untested paths more than that this change in behavior is doing the right thing. 
Hi, I have another question about this, in D10979 which I mentioned previously, the author wrote something similar: `normalizeEdgeWeights` which also scale the sum to be within UINT32_MAX. But later in D15519 he changed all of weight to probability. I guess he had changed to this because of probably the same reason as you mentioned, and with probability we do not need to care about integer overflow.
However, currently no other place in SimplifyCFG is doing CFG fix using probability, all of them are integer weight calculation. The most typical one is `performBranchToCommonDestFolding`. So I'm concerned about consistency, may I ask is that okay to do probability calculation here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131287