[PATCH] D98898: [SimplifyCFG] use profile metadata to refine merging branch conditions

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2861-2862
+        BranchProbability::getBranchProbability(TWeight, TWeight + FWeight);
+    Likely = BranchProbability::getBranchProbability(
+        LikelyBranchWeight, LikelyBranchWeight + UnlikelyBranchWeight);
+  }
----------------
spatel wrote:
> lebedev.ri wrote:
> > And now that i actually start reviewing this, i see what was irking me.
> > While it seems like we should be using the weights from `LowerExpectIntrinsic`,
> > why is that the right threshold for profile-driven weights?
> > 
> > There's `TLI->getPredictableBranchThreshold()`,
> > shouldn't we use that?
> > 
> > Because the `LikelyBranchWeight`/`UnlikelyBranchWeight` appears to only be used in 
> > `CodeGenFunction.cpp` and `LowerExpectIntrinsic.cpp`, none of the transforms use them,
> > unlike `TLI->getPredictableBranchThreshold()`. Which to me looks like
> > that they are `LowerExpectIntrinsic`'s implementation detail
> > that is unintentionally overexposed.
> Yes, I thought about that too, and I don't have a good reason for using likely/unlikely. I think we will want to move the threshold API to TTI (rather than TLI) if we go with that setting, so we're not making an optimizer pass depend on TLI unnecessarily. 
I'll revert D98945 along with fixing rG08196e0b2e1f8aaa8a854585335c17ba479114df in a moment.


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

https://reviews.llvm.org/D98898