diff mbox series

[v4,6/6] qht: Fix threshold rate calculation

Message ID 20200617201309.1640952-7-richard.henderson@linaro.org
State Accepted
Commit 06c4cc3660b366278bdc7bc8b6677032d7b1118c
Headers show
Series Vs clang-10 and gcc-9 warnings | expand

Commit Message

Richard Henderson June 17, 2020, 8:13 p.m. UTC
tests/qht-bench.c:287:29: error: implicit conversion from 'unsigned long'
  to 'double' changes value from 18446744073709551615
  to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
        *threshold = rate * UINT64_MAX;
                          ~ ^~~~~~~~~~

Fix this by splitting the 64-bit constant into two halves,
each of which is individually perfectly representable, the
sum of which produces the correct arithmetic result.

Cc: Emilio G. Cota <cota@braap.org>
Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
Question: Should we really be scaling by UINT64_MAX?

The comparisons against info->r mean that 1.0 is exceedingly unlikely
to hit.  Or if that is supposed to be the point, why is is the test

  r >= threshold
not
  r > threshold

where, if threshold == UINT64_MAX, there is zero chance of the
test being true for 1.0.
---
 tests/qht-bench.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

Comments

Richard Henderson June 17, 2020, 8:58 p.m. UTC | #1
On 6/17/20 1:13 PM, Richard Henderson wrote:
> -        *threshold = rate * UINT64_MAX;

> +        *threshold = (rate * 0xffff000000000000ull)

> +                   + (rate * 0x0000ffffffffffffull);


Well, while this does silence the warning, it produces the exact same results
as before, since the intermediate double result still only has 53 bits of
precision.

We're probably be better off with the nextafter(0x1p64, 0.0) form that we've
used elsewhere.


r~
Peter Maydell June 19, 2020, 5:31 p.m. UTC | #2
On Wed, 17 Jun 2020 at 21:59, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 6/17/20 1:13 PM, Richard Henderson wrote:

> > -        *threshold = rate * UINT64_MAX;

> > +        *threshold = (rate * 0xffff000000000000ull)

> > +                   + (rate * 0x0000ffffffffffffull);

>

> Well, while this does silence the warning, it produces the exact same results

> as before, since the intermediate double result still only has 53 bits of

> precision.

>

> We're probably be better off with the nextafter(0x1p64, 0.0) form that we've

> used elsewhere.


Yeah, the code does look a bit odd. I'm going to apply this one to master
as well just to suppress all the patchew nagmails, but we should
figure out the correct thing to do here.

thanks
-- PMM
diff mbox series

Patch

diff --git a/tests/qht-bench.c b/tests/qht-bench.c
index e3b512f26f..eb88a90137 100644
--- a/tests/qht-bench.c
+++ b/tests/qht-bench.c
@@ -284,7 +284,8 @@  static void do_threshold(double rate, uint64_t *threshold)
     if (rate == 1.0) {
         *threshold = UINT64_MAX;
     } else {
-        *threshold = rate * UINT64_MAX;
+        *threshold = (rate * 0xffff000000000000ull)
+                   + (rate * 0x0000ffffffffffffull);
     }
 }