[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