[PATCH] D158680: RFC: Interpet {branch_weights 1, 0} as likely/unlikely

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> Meaning !{"branch_weights", i32 1, i32 0} being an abstract representation of a "likely" branch.

In general, an abstract representation should not conflict with a valid concrete representation..

> The adjustments for the concrete weights of likely/unlikely are moved into BranchProbabilityInfo and are applied when computing the analysis.

BPI should be an analysis that only consumes branch weights, and I don't particularly like the idea of, oh btw, it will try to canonicalize branch weights when it runs.

If we want to canonicalize likely/unlikely representation for branch weights, we should probably do it elsewhere and make sure it's created that way rather than trying to lazily fix things up on when it is used.

Proper canonicalization is probably a much bigger change, so the next question is, is it worth it?

I can see that a {X,0} and {1,0} are equivalent in terms of branch probabilities, and the use of branch metadata in `BranchProbabilityInfo::calcMetadataWeights` also confirms that. Is there any practical purpose of this normalization other than clean things up?

There is a subtle difference between {X,0} and {1,0}. A lot of heuristic in PGO likes to +1 to everything to avoid 0s, and {X+1,1} vs {2,1} can be hugely different. Examples are: `SampleProfileLoader::generateMDProfMetadata`, and `BlockFrequencyInfoImplBase::addToDist` (though this one should not be impacted by this change as it's on BFI layer)

The other thing is there are some existing inconsistencies as to whether we should care about absolute values of branch weights or not.

1. For branch instructions, we care about only the distribution, but not the values; for calls however, we do care about the absolute values. And for sample pgo, we annotate branch weights with actual counts, even though the values don't change BPI result. Many people working in sample PGO like to think about branch weights values as a good reference, but their absolute values may not always be meaningful. I know that may have diverged from what the llvm docs say, but that's the status quo.

2. We also don't attempt to normalize {X*Y, Z*Y} to {X, Z}. If we don't do that, do we actually care about {X,0} and {1,0}, which is special form of the former.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158680