[PATCH] D11442: Provide an interface normalizeSuccWeights in MachineBasicBlock to normalize its successors' weights and use it in other places.

[Xinliang David Li]

On Wed, Aug 5, 2015 at 4:22 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Aug-05, at 14:16, Xinliang David Li <davidxl at google.com> wrote:
>>
>>>
>>> If you go ahead with this, then you should first change
>>> `BranchProbability` to use this design, and then you can reuse that
>>> class for branch weights.
>>
>> I think BranchProbability (defined in
>> llvm/Support/BranchProbability.h) is well suited as a data structure
>> used in interface. It is the underlying 'weight' storage in
>> BranchProbabilityInfo and MBPI needs the change.
>
> IIUC, you want the underlying weight to be treated like a
> probability (represented in fixed point),

yes.

> since BranchProbability isn't convenient for doing math.

but not for this reason. Currently, branch probability is represented
as weight over weight sum. The weight  can be scaled arbitrarily. Once
the weights of a BB gets normalized, the weight of an edge previous
cached will become invalidated.  What I suggest is to attach a clear
meaning to weight aka, branch probability. With this change, the
getter interfaces be backward compatible with the old weight
representation. In the longer term we can totally remove 'weight' from
interface.  Another good side effect is that it makes
getEdgeProbability fast without resorting to getSumBlock. It also
makes profile update easier to reason.


> So I'm suggesting we change BP to
> be convenient for doing math (by making it a fixed-point number, or
> otherwise adding math operations), and change BPI to return
> BranchProbabilities instead of weights.

I agree we should change BPI to use BP, not weights (though their
meaning is the same). Whether we should change BP to use fixed point
representation depends on how fancy the math support we want from it.
It seems to be current BP interfaces are good enough.

>
> Maybe weights should be fixed-point probabilities as well?

yes -- that is the core of my suggestion.

> But I
> think you should fix BranchProbability first.

I am ok with that, but not sure it should be done first.

>
> Or are you saying that it's convenient somewhere that
> BranchProbability is a ratio?  We could expose both ratios and
> fixed point probabilities from BPI.  But they're both probabilities
> (the "weights" terminology doesn't really make sense).

I don't see a strong reason to expose fixed point prob from BPI. The
ratio interface seems good enough. The implementation of
getEdgeProbability simply converts the fixed point rep into a ratio
(current BP form).

>
>> For BranchProbability, I also think we need to generalizes it as
>> ValueRatio class and let BranchProbablity derives from it. I see many
>> cases that BranchProbability is simply used a convenient class to
>> present ratio.
>
> BranchProbability is a little more restricted than just a `Ratio`,
> since it disallows ratios larger than 1.  But yes, if other users
> are using BP for general ratios, we should split that out into a
> Ratio class (regardless of whether BranchProbability gets changed).

yes.

>
>>> BTW, another approach would be to change `getSumForBlock()` to return a
>>> 64-bit number, and just use 64-bit math everywhere (weights would still
>>> be 32-bit).  Sounds like a lot less work.  Thoughts on that?
>>>
>>
>> yes -- this sounds like less work -- but I think 'getSumForBlock()' is
>> really an internal interface that should not be exposed to clients.
>
> Yeah, maybe it shouldn't be exposed.  We could change BPI (and MBPI)
> to expose only probabilities.

yes.

thanks,

David

>