diff mbox series

bug fix for hp-timing.h (aarch64)

Message ID 8ae95272accb441aad774a5f10d27d57@amazon.com
State Superseded
Headers
Series bug fix for hp-timing.h (aarch64) |

Checks

Context Check Description
dj/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
dj/TryBot-32bit fail Patch series failed to apply

Commit Message

develop--- via Libc-alpha Jan. 11, 2023, 4:44 p.m. UTC 
  On Graviton 3 the frequency is 1050000000 and the original function hp-timing.h no longer gives the correct answer. The following patch can fix the bug:

```

I have also submitted a bug report with the proposed changes at -
https://sourceware.org/bugzilla/show_bug.cgi?id=29329

Regards,

Jun

Comments

Zack Weinberg Jan. 11, 2023, 5:22 p.m. UTC | #1 
On Wed, Jan 11, 2023, at 11:44 AM, Tang, Jun wrote:
> On Graviton 3 the frequency is 1050000000 and the original function 
> hp-timing.h no longer gives the correct answer. The following patch can 
> fix the bug:

Please try this instead:

> -   (Diff) = ((End) - (Start)) * (UINT64_C(1000000000) / freq); \
> +   (Diff) = (((End) - (Start)) * UINT64_C(1000000000)) / freq; \

This avoids using floating-point arithmetic but should still do the rescale correctly.
(The multiplier and divisor are both guaranteed to be less than 2**32, so, as long as we do the multiplication in 64-bit arithmetic, we cannot lose bits via overflow.)

zw
develop--- via Libc-alpha Jan. 31, 2023, 2:47 p.m. UTC | #2 
Zack,

After rounds of discussion, the suggested solution is the best option and works for all existing benches.  Let me know if I need to submit another request to get this merged

> -   (Diff) = ((End) - (Start)) * (UINT64_C(1000000000) / freq); \
> +   (Diff) = (((End) - (Start)) * UINT64_C(1000000000)) / freq; \

Regards,

Jun

-----Original Message-----
From: Zack Weinberg <zack@owlfolio.org> 
Sent: Wednesday, January 11, 2023 11:22 AM
To: GNU libc development <libc-alpha@sourceware.org>
Subject: Re: bug fix for hp-timing.h (aarch64)

On Wed, Jan 11, 2023, at 11:44 AM, Tang, Jun wrote:
> On Graviton 3 the frequency is 1050000000 and the original function 
> hp-timing.h no longer gives the correct answer. The following patch 
> can fix the bug:

Please try this instead:

> -   (Diff) = ((End) - (Start)) * (UINT64_C(1000000000) / freq); \
> +   (Diff) = (((End) - (Start)) * UINT64_C(1000000000)) / freq; \

This avoids using floating-point arithmetic but should still do the rescale correctly.
(The multiplier and divisor are both guaranteed to be less than 2**32, so, as long as we do the multiplication in 64-bit arithmetic, we cannot lose bits via overflow.)

zw
diff mbox series

Patch

diff --git a/sysdeps/aarch64/hp-timing.h b/sysdeps/aarch64/hp-timing.h
index c8469e3011..c5a1204d2c 100644
--- a/sysdeps/aarch64/hp-timing.h
+++ b/sysdeps/aarch64/hp-timing.h
@@ -41,7 +41,7 @@  typedef uint64_t hp_timing_t;
#define HP_TIMING_DIFF(Diff, Start, End)                       \
({  hp_timing_t freq;                                          \
     __asm__ __volatile__ ("mrs %0, cntfrq_el0" : "=r" (freq)); \
-   (Diff) = ((End) - (Start)) * (UINT64_C(1000000000) / freq); \
+   (Diff) = (hp_timing_t)((End) - (Start)) * ((1000000000.0) / freq);  \
})
 #endif /* hp-timing.h */
```