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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D98898#2640239 <https://reviews.llvm.org/D98898#2640239>, @lebedev.ri wrote:

> Can prof metadata and `MD_unpredictable` be mixed together?
> What should we do here in presence of `MD_unpredictable`?

They can be mixed, but I don't see any precedence for dealing with them simultaneously. So we get to decide here :) -- or in a follow-up standalone patch to make it more definitive
In theory, either of these metadata could be attached manually by a programmer or from actual training data / profile-guided optimization (PGO). 
I'm leaning towards having `unpredictable` be the winner in that case because branch weights are good, but they can't really tell us how a given target will behave since we can't model even basic branch history hardware here. So we have to defer to whoever/whatever said a branch was unpredictable as true/correct.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2861-2862
+        BranchProbability::getBranchProbability(TWeight, TWeight + FWeight);
+    Likely = BranchProbability::getBranchProbability(
+        LikelyBranchWeight, LikelyBranchWeight + UnlikelyBranchWeight);
+  }
----------------
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. 


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

https://reviews.llvm.org/D98898