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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth 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);
----------------
LukeZhuang wrote:
> 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?
Looks like I may have steered you in the wrong direction. BPI pass is going to scale these anyway. see https://github.com/llvm/llvm-project/blob/27d666b9adc12a13f67ccde96d5b4d181f56ffc5/llvm/lib/Analysis/BranchProbabilityInfo.cpp#L420

We should just do what they do, and since this is now a common functionality, we should extract that logic and make it reusable. `ProfDataUtils` is a good candidate for where that code should live, since this is about branch weights, but if there is somewhere else that makes sense, that is also fine. I'd also suggest making that a separate patch, since its kind of a code clean up that is orthogonal to this change. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131287