[PATCH] Fix information loss in branch probability computation.

Duncan P. N. Exon Smith dexonsmith at apple.com
Thu May 7 08:39:58 PDT 2015


> On 2015 May 7, at 10:30, Diego Novillo <dnovillo at google.com> wrote:
> 
> On Wed, May 6, 2015 at 4:18 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
> 
>> The new assertion doesn't check the sum though, and the sum can now
>> overflow because of the floor.
> 
> Oh, right. Silly me.
> 
>> I think if you want to go this way, you need to:
>> 
>> 1. Check that sum in use is small enough after adding 1 to all the
>>    weights.
>> 2. If it overflows, rescale everything locally.
>> 
>> It's probably simpler just to split the two changes in BPI into two
>> patches, or roll in the change to `visitSwitch()`.
> 
> Sorry. I'm not sure I follow you here. If I split the changes to BPI
> from the change in visitSwitch(), I'll run into the existing assertion
> in visitSwitch() (one of the tests fails). Or do you mean, rolling in
> the algorithmic changes to visitSwitch()?

Sorry for not being clear.

I see three ways to stage this without knowingly introducing bugs.  In
order of simplicity:

 1. Do everything at once.  Rewrite `visitSwitch()` in the same commit
    as this patch.
 2. Split BPI change into two, and change `visitSwitch()` in the middle:
    (a) Change BPI to stop flooring weights at 1 (allow weights at 0).
        Still guarantee weights smaller than UINT32_MAX/NumSuccessors.
    (b) Change `visitSwitch()` to stop flooring weights at 1 and work
        correctly with weights of 0.
    (c) Change BPI to stop guaranteeing weights smaller than
        UINT32_MAX/NumSuccessors.
 3. Don't split BPI; instead, rescale in `visitSwitch()`.
    (a) Change BPI to stop flooring weights at 1 and to stop
        guaranteeing weights smaller than UINT32_MAX/NumSuccessors, and
        in the same commit, change `visitSwitch()` to rescale weights
        locally when flooring at 1 causes the sum to overflow.
    (b) Change `visitSwitch()` to stop flooring weights at 1 and work
        correctly with weights of 0, and remove the local code to
        rescale.

The current patch looks like #3(a) but without rescaling, so I was
describing how to finish it up.

My next comment said: I think #2 is a simpler way to roll this out than
#3.

#1 is simplest, but you already rejected it since it'll make it harder
to triage any problems it introduces.



More information about the llvm-commits mailing list