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

[Duncan P. N. Exon Smith]

> On 2015-Aug-05, at 14:47, Cong Hou <congh at google.com> wrote:
> 
> On Wed, Aug 5, 2015 at 1:59 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> On 2015-Aug-05, at 13:48, Cong Hou <congh at google.com> wrote:
>>> 
>>> On Wed, Aug 5, 2015 at 12:24 PM, Duncan P. N. Exon Smith
>>> <dexonsmith at apple.com> wrote:
>>>> 
>>>> In terms of changing branch weights to use fixed point arithmetic (and
>>>> always be normalized): I like this idea.  I'd prefer a number like
>>>> UINT32_MAX instead of 100000, but I agree with David that 32 bits ought
>>>> to be enough.  Something similar to the `BlockMass` design seems about
>>>> right.
>>>> 
>>>> The major downside is that passes that modify the CFG will always have
>>>> to update edge weights to renormalize them.  Currently, if a pass
>>>> removes a successor edge, it needn't worry about normalization at all.
>>>> Although that's convenient, it's probably not important.  (Or maybe
>>>> they could be normalized lazily...?)
>>> 
>>> One important idea from David's design is that we should pass branch
>>> probability instead of edge weight when adding new edges or updating
>>> edge weights. If we decide to use this design, then when edge weights
>>> (or probabilities) are updated, we should always update the weights on
>>> other out-edges from the same block. Removing edge doesn't need
>>> normalization but if we want a fast getEdgeProbability()
>>> implementation without storing the sum of edge weights, we should
>>> always keep edge weights normalized.
>> 
>> Wouldn't caching in BPI be fast enough for `getEdgeProbability()`?
>> And it slows down update speed (since, as you point out, every update
>> to one edge requires updates to all the others).
> 
> As most branches are two-way, I think updating edge weights when
> removing an edge (so that mostly there are at most one branch left)
> should be very cheap. Here I think we are talking about MBPI, which
> doesn't cache anything (BPI doesn't have the issue we are discussing
> as its cached weights are guaranteed to be normalized). The only way
> MBPI can get edge weight or probability is by reading info from
> MachineBasicBlock class.
> 
>> 
>>>> 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.
>>> 
>>> OK. I think we can allow BranchProbability to be built either from
>>> uint32_t or BlockMass-like class.
>> 
>> I was suggesting: change `BranchProbability` to be a BlockMass-like
>> class.  (Or are we on the same page already?)
> 
> OK. I see what you mean: I thought we are going to use a
> BlockMass-like class in BranchProbability just like how
> BlockFrequencyInfoImplBase uses BlockMass. You are right:
> BranchProbability can be written as BlockMass.
> 
>> 
>>>> 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?
>>> 
>>> As BranchProbability only accepts uint32_t parameters, we still need
>>> to scale down the sum of 64-bit type when constructing probabilities.
>> 
>> Sure, but BPI could easily cache that result.  It caches the weight
>> right now, and could just as easily cache the probability instead (or
>> as well).
> 
> As above, MBPI doesn't cache anything at this point. Should we let it
> cache edge probabilities?
> 

I don't see why not.  Maybe this should be *instead* of caching edge
weights.