[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

[Wenlei He via Phabricator via cfe-commits]

wenlei added a comment.

In D158668#4611988 <https://reviews.llvm.org/D158668#4611988>, @MatzeB wrote:

> And as another strawman / discussion-starter I put up D158680 <https://reviews.llvm.org/D158680> where I use `!{"branch_weights", i32 1, i32 0}` to represent likely branches and the actual "LikelyWeight" mostly becomes an internal implementation detail of the BranchProbabilityInfo class... This seemed like a neat way to get an abstract "likely", "unlikely" notation, but not sure of the effects if we no longer would have "truly zero" weights because they would be interpreted differently now...

Commented on that patch, I'm not convinced that canonicalize {X,0} -> {1,0} is a good idea mostly because: 1) we don't attempt to canonicalize {X*Z,Y*Z} -> {X,Y}; 2) there are places where the absolute values matters; 3) canonicalize branch_weights as a side effect of BPI seems not right.

> Remove -unlikely-branch-weight option and just use 1.

Seems fine, I don't have a strong opinion.

> Add getLikelyBranchWeight() function returning the value of the -likely-branch-weight option defaulting to 2000.

This makes sense to me.

> prefer pass-specific weights rather than a unified notion of "likely"/"unlikely"... So with the latest round of patches it's probably best to abandon this for now.

I don't see them as mutually exclusive. In fact when I look at your other patches, I feel that we need a default likely/unlikely for cases where we just don't have a good answer, hence fall back to default. So the default likely/unlikely probability is a layer below pass-specific setting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158668