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

[Cong Hou]

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?


thanks,
Cong