[PATCH] D12603: Use fixed-point representation for BranchProbability
Cong Hou via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 24 15:03:43 PDT 2015
Number of test failures with D/2 and without D/2: 19 vs 18.
On Thu, Sep 24, 2015 at 2:50 PM, Xinliang David Li <davidxl at google.com>
> Without modifying any test cases, how many failures are there with D/2 vs
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits