-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Has the volatile declaration in fsum() outlived its usefulness? #100833
Comments
It certainly seems worth attempted removal: I'd say remove the qualifier, run the buildbots, and see what happens. Incidentally, this has little to do with x87 (the "this prevents double rounding" text in the comment is inaccurate). It's a defence against the compiler ignoring the temporary assignment to |
For the record, here's an example of how double-rounding can break the contract of 2SUM, Suppose that So our first 2SUM output Supporting Python code: >>> from fractions import Fraction as F
>>> x = float.fromhex('0x1.0000000000001p+63')
>>> y = float.fromhex('0x1.ffc0000000001p+9')
>>> # x + y is in the binade [2**63, 2**64), so rounding
>>> # to 64-bit precision is equivalent to rounding to the nearest int
>>> hi64 = round(F(x) + F(y)) # round to 64-bit precision
>>> hi = float(hi64) # then back to 53-bit precision - double rounding!
>>> hi.hex()
'0x1.0000000000002p+63'
>>> lo = F(x) + F(y) - F(hi) # value that would be needed to satisfy x + y == hi + lo
>>> lo == float(lo) # but it's not representable as a float
False
>>> lo.numerator.bit_length()
54 |
|
The buildbots seem happy; I've marked #100845 as ready for review. |
This PR removes the `volatile` qualifier on various intermediate quantities in the `math.fsum` implementation, and updates the notes preceding the algorithm accordingly (as well as fixing some of the exsting notes). See the linked issue #100833 for discussion.
This bit of defensive programming is costly. Removing the
volatile
declaration reduces the running time offsum()
for a hundred floats from2.26 usec per loop
to1.42 usec per loop
.I'm thinking the x87 issues have mostly faded. If we do need to keep this, can it be wrapped in an
#ifdef
so that we don't have everyone paying for a problem that almost nobody has?Linked PRs
The text was updated successfully, but these errors were encountered: