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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
