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

[Xinliang David Li]

On Thu, Jul 30, 2015 at 10:55 AM, Cong Hou <congh at google.com> wrote:
> congh added a comment.
>
> In http://reviews.llvm.org/D11442#215298, @davidxl wrote:
>
>> In general, I find doing normalization like this is error prone. Once the weights gets normalized, the new weight added in later may become meaningless and there is no good way to check against it.
>
>
> Yes, but this depends on when the normalization is applied. If the user calls it explicitly, he/she should be aware that the edge weights are already changed so the newly added one needs to be adjusted. Otherwise, the user also needs to check the current normalized weights to determine the new successor's weight (otherwise how to assign weight to a new successor?). But this design can still bring potential errors.

Current interfaces are hard for users to use (need to be aware of
implicit assumptions etc) and we can not rely on them not making
mistakes.

The right way is to redesign APIs such there is no ambiguity and no
chance for mistakes.  Mistakes that leads to wrong weight is hard to
catch by the verifier and performance regressions introduced are even
harder to triage as you have noticed.

David

>
>> I suggest using this opportunity to use edge weight to represent fixed point representation of the branch probability (aka scaled probability). We can pick a large scale factor such as 100000. It has the nice property that the sum of all 'weights' will guarantee to be always '100000'.  Another nice side effect is that the computation of getEdgeProbability is now super fast -- there is no need to keep the getSumWeight interface any more.
>
>
> I agree this design: the only use of weights is to compute edge probabilities. We should adjust the edge "weight" in terms of probabilities instead of an absolute weight. This is a more flexible representation.
>
>> The addSuccessor interface will also need to be modified such that instead of passing raw 'weight', it needs to pass in BranchProbablity(N,M).
>
>
> So the probabilities of old successors can be adjusted accordingly.
>
>> In the longer term, we should remove uses of getEdgeWeight interface completely and replace it with getEdgeProbabity instead (as it is already efficient). Weight will simply be an implementation detail that does not need to be exposed to users.
>
>
>
> http://reviews.llvm.org/D11442
>
>
>