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

[Duncan P. N. Exon Smith]

(resending to the actual mailing list)

> On 2015-Aug-03, at 15:03, Cong Hou <congh at google.com> wrote:
> 
> Duncan, could you please take a look at this patch and the discussion
> here? What is your suggestion?
> 

Thanks for the ping; somehow this fell off my radar.

The patch LGTM with some nitpicks (see below).

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...?)

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.

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?

> Index: include/llvm/CodeGen/MachineBasicBlock.h
> ===================================================================
> --- include/llvm/CodeGen/MachineBasicBlock.h
> +++ include/llvm/CodeGen/MachineBasicBlock.h
> @@ -64,6 +64,11 @@
>   Instructions Insts;
>   const BasicBlock *BB;
>   int Number;
> +
> +  /// SuccWeightsNormalized - A flag tracking whether the weights of all

New style leaves out the "SuccWeightsNormalized -" in the comment.  You
should probably do an initial NFC commit that updates the comment style
in the file, and then in your commit you can just use the new style.

> +  /// successors are normalized.
> +  bool SuccWeightsNormalized;

`bool`s should use Boolean language, something like "is" or "has" or
"does" or something.

Maybe "AreSuccWeightsNormalized"?

> +
>   MachineFunction *xParent;
> 
>   /// Predecessors/Successors - Keep track of the predecessor / successor
> @@ -137,6 +142,10 @@
>   const MachineFunction *getParent() const { return xParent; }
>   MachineFunction *getParent() { return xParent; }
> 
> +  /// succWeightsNormalized - Return whether all weights of successors are
> +  /// normalized.
> +  bool succWeightsNormalized() const { return SuccWeightsNormalized; }

Same for the accessor.  "areSuccWeightsNormalized()"?

> +

Extra newline?

> 
>   /// bundle_iterator - MachineBasicBlock iterator that automatically skips over
>   /// MIs that are inside bundles (i.e. walk top level MIs only).
> @@ -398,6 +407,12 @@
>   /// Set successor weight of a given iterator.
>   void setSuccWeight(succ_iterator I, uint32_t weight);
> 
> +  /// Normalize all succesor weights so that the sum of them does not exceed
> +  /// UINT32_MAX. Return true if the weights are modified and false otherwise.
> +  /// Note that weights that are modified after calling this function are not
> +  /// guaranteed to be normalized.
> +  bool normalizeSuccWeights();
> +
>   /// removeSuccessor - Remove successor from the successors list of this
>   /// MachineBasicBlock. The Predecessors list of succ is automatically updated.
>   ///
> 


> Thank you very much!
> 
> 
> Cong
> 
> 
> On Thu, Jul 30, 2015 at 11:18 AM, Xinliang David Li <davidxl at google.com> wrote:
>> On Thu, Jul 30, 2015 at 11:12 AM, Cong Hou <congh at google.com> wrote:
>>> On Thu, Jul 30, 2015 at 10:23 AM, David <davidxl at google.com> wrote:
>>>> davidxl added a comment.
>>>> 
>>>> 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.
>>>> 
>>>> 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.
>>> 
>>> BTW, maybe we can use BlockMass as the fixed point representation or
>>> borrow its design. It already has nice interfaces like +/-/*
>>> operations.
>> 
>> 
>> I think 32bit integer is good enough. Please wait for Duncan's
>> feedback in general.
>> 
>> David
>> 
>> 
>>> 
>>>> 
>>>> The addSuccessor interface will also need to be modified such that instead of passing raw 'weight', it needs to pass in BranchProbablity(N,M).
>>>> 
>>>> 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.
>>>> 
>>>> 
>>>> ================
>>>> Comment at: lib/CodeGen/IfConversion.cpp:1245
>>>> @@ -1242,3 +1244,3 @@
>>>>    BBCvt = MBPI->getEdgeWeight(BBI.BB, CvtBBI->BB);
>>>> -    SumWeight = MBPI->getSumForBlock(CvtBBI->BB, WeightScale);
>>>> +    SumWeight = MBPI->getSumForBlock(CvtBBI->BB);
>>>>  }
>>>> ----------------
>>>> why not move this before getEdgeWeight so that the explicit call to normalize weight is not needed?
>>>> 
>>>> 
>>>> http://reviews.llvm.org/D11442
>>>> 
>>>> 
>>>>