<div dir="ltr">Without modifying any test cases, how many failures are there with D/2 vs without?<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 24, 2015 at 2:43 PM, Cong Hou <span dir="ltr"><<a href="mailto:congh@google.com" target="_blank">congh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">congh added inline comments.<br>
<span class=""><br>
================<br>
Comment at: lib/Support/BranchProbability.cpp:98<br>
@@ -59,3 +97,3 @@<br>
   Rem = ((Rem % D) << 32) | Lower32;<br>
-  uint64_t LowerQ = Rem / D;<br>
+  uint64_t LowerQ = (Rem + D / 2) / D;<br>
   uint64_t Q = (UpperQ << 32) + LowerQ;<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > Should D/2 be added to ProductLow above (and check overflow and carry bit)?<br>
> > ><br>
> > > 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.<br>
> > It is the division by D that may cause precision loss and I think this is the proper place to add D/2.<br>
> ><br>
> > 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.<br>
> 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)<br>
</span>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.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D12603" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12603</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>