[PATCH] D12603: Use fixed-point representation for BranchProbability

[Cong Hou via llvm-commits]

Number of test failures with D/2 and without D/2: 19 vs 18.


thanks,
Cong

On Thu, Sep 24, 2015 at 2:50 PM, Xinliang David Li <davidxl at google.com>
wrote:

> Without modifying any test cases, how many failures are there with D/2 vs
> without?
>
> David
>
> On Thu, Sep 24, 2015 at 2:43 PM, Cong Hou <congh at google.com> wrote:
>
>> congh added inline comments.
>>
>> ================
>> Comment at: lib/Support/BranchProbability.cpp:98
>> @@ -59,3 +97,3 @@
>>    Rem = ((Rem % D) << 32) | Lower32;
>> -  uint64_t LowerQ = Rem / D;
>> +  uint64_t LowerQ = (Rem + D / 2) / D;
>>    uint64_t Q = (UpperQ << 32) + LowerQ;
>> ----------------
>> davidxl wrote:
>> > congh wrote:
>> > > davidxl wrote:
>> > > > Should D/2 be added to ProductLow above (and check overflow and
>> carry bit)?
>> > > >
>> > > > Besides, independent on what scale factor to use for BP,  I think
>> this change should not be included in this patch. It changes the rounding
>> behavior of the scale() method (instead of truncating) which is irrelevant
>> to the main purpose of the patch. This should be done as a follow up patch
>> if it is desirable.
>> > > It is the division by D that may cause precision loss and I think
>> this is the proper place to add D/2.
>> > >
>> > > It is OK to separate this change to another patch (if it is
>> necessary). This will cause many test failures and I will fix them as long
>> as this patch is approved.
>> > I would expect more test failures with this particular change as it
>> differs from existing implementation.  How many more tests fail without
>> this change? (note that the rounding in the BP construction is still needed
>> and kept)
>> There are three regression test failures if we remove D/2 here and two
>> are caused by changes on test cases in this patch. That is two say, without
>> D/2 we only have one more test failure. Other failures are branch
>> probability unit tests.
>>
>>
>> http://reviews.llvm.org/D12603
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150924/d6c174bf/attachment.html>