[PATCH] D12603: Use fixed-point representation for BranchProbability
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 24 14:50:42 PDT 2015
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