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