[PATCH] Allow 0-weight branches in BranchProbabilityInfo.

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed May 6 11:27:04 PDT 2015


> On 2015 May 6, at 14:26, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015 May 6, at 14:20, Hans Wennborg <hans at chromium.org> wrote:
>> 
>> On Wed, May 6, 2015 at 10:58 AM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>> 
>>>> On 2015 May 6, at 13:50, Diego Novillo <dnovillo at google.com> wrote:
>>>> 
>>>> On Wed, May 6, 2015 at 10:46 AM, Duncan P. N. Exon Smith
>>>> <dexonsmith at apple.com> wrote:
>>>>> 
>>>>>> On 2015 May 6, at 13:39, Diego Novillo <dnovillo at google.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 05/06/15 13:37, Hans Wennborg wrote:
>>>>>>> On Wed, May 6, 2015 at 10:09 AM, Duncan P. N. Exon Smith
>>>>>>> 
>>>>>>> Are you saying you don't want to guarantee that anymore, or that
>>>>>>> forcing each weight <= UINT32_MAX / SI.getNumSuccessors() is
>>>>>>> unnecessarily strict?
>>>>>> 
>>>>>> Yeah, I've wanted to remove this restriction for some time. The only requirement we really need is that the sum of all the weights fits within 32 bits.
>>>>>> 
>>>>>> I'm going to be removing this restriction in http://reviews.llvm.org/D9442
>>>>> 
>>>>> You'll have to leave it in place until the downstream consumers stop
>>>>> flooring the weights at 1, but as long as there's a plan for lifting
>>>>> the restriction I'm happy.
>>>> 
>>>> Well, currently BPI wants each branch weight to fit UINT32_MAX /
>>>> successors, but in D9442, I will implement your suggestion of fitting
>>>> weights such that Sum(weights) < UINT32_MAX.  This assert() may start
>>>> triggering after my change.
>>> 
>>> Well, it looks to me like this `assert()` *will* start triggering; maybe
>>> not with any checked in testcases, but we know how to trigger it and it's
>>> not hard.
>> 
>> We could just drop this assert, or change it. It's unnecessarily
>> strict, as that thing that really matters is that the sum of weights
>> doesn't overflow.
> 
> As long as we're flooring to 1, this assert is necessary.  Adding 1 to
> all the weights increases the sum, but this assert guarantees that it
> doesn't increase it beyond `UINT32_MAX`.
> 

(Or we otherwise need to check that we have space to add 1 to all the
weights of 0.)



More information about the llvm-commits mailing list