[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 1:18 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> (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...?)

I wonder if that will be an issue in reality. If an edge proves to be
dead statically, its branch probability (with PGO) will be zero. This
of course requires case-by-case checking. SCCP is a pass that does
this transformation -- but it is ok there are it simply converts the
conditional branch into unconditional branch which simply discards the
prof data (useless).

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

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.

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

thanks,

David


>> 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
>>>>>
>>>>>
>>>>>
>
>