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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

In D131287#3744258 <https://reviews.llvm.org/D131287#3744258>, @LukeZhuang wrote:

> Hi @paulkirth, sorry for the late reply. If I understand it correctly, BlockFrequencyInfo can only be used with profile information. I have found the original author of updateBlockFreqAndEdgeWeight(https://reviews.llvm.org/D10979 and https://reviews.llvm.org/D15519), that's why the author added `if (HasProfileData)` and only do the fix when have profile data.

I'm not sure that was the rationale looking at the original patch. I assume it was just because branch weights were assumed to not be present if profiling wasn't enabled.  The bigger issue is that I think BPI and BFI passes assume loops are simplified, which I //think// happen in this pass. https://github.com/llvm/llvm-project/blob/f24c299e7d733c0afb9a17655201f37a43d84ce5/llvm/lib/Analysis/BlockFrequencyInfo.cpp#L9

> However, this code is 7 years ago and we added __builtin_expect_with_probability in 2020. This probability issue(that example at the beginning of my description) can also happen even without profile data, the user will find the probability of executing `bar` is not expected. So it’s not sufficient for us to only do this with profile data. This is our concern about using BlockFrequencyInfo.
> And again thank you very much for the kind and useful comments! I think all of them are reasonable and is working on it.

branch weights are how we derive probability and frequency information. `__builtin_expect_with_probability` still adds weights, it just adds them using the defined probability distribution. https://github.com/llvm/llvm-project/blob/f24c299e7d733c0afb9a17655201f37a43d84ce5/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp#L63

As mentioned above, the bigger issue is that the BPI and BFI passes may be incompatible w/ simplifyCFG.  You may not need to run the passes though. I would investigate whether you can construct/instantiate the BranchProbability/BlockFrequency directly given the input blocks and still derive the result you're after.  Do note though, I haven't looked at the BPI/BFI passes, their algorithms, or the classes deeply, so this may not help. It's just that for correctly balancing/scaling weights I doubt you would need to run the full passes. That's just a suspicion though, and I may be steering you the wrong way. It's just what I would investigate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131287