Uh oh!
There was an error while loading. Please reload this page.
gh-73468: Add math.fma() function - #116667
Conversation
vstinner
commented
Mar 12, 2024
Previous attempt in 2020: #17987 |
Uh oh!
There was an error while loading. Please reload this page.
e988215 to
3198308Comparebedevere-bot
commented
Mar 13, 2024
vstinner
commented
Mar 13, 2024
For WASI, I skipped tests on the zero sign. It sounds too complicated to me to work around an implementation which doesn't implement IEEE 754-2008 correctly. Maybe we should even remove the function on this platform? |
test_math failed on FreeBSD and FreeBSD 14. test_importlib failed on RHEL7 Refleaks, Ubuntu NoGIL Refleaks, and MacOS M1 Refleaks NoGIL. But it's a known and unrelated leak: #116731 |
vstinner
commented
Mar 13, 2024
@mdickinson: Would you mind to review this PR? I had to skip test_fma_zero_result() on FreeBSD and WASI platforms, because their libc fma() doesn't compute the zero sign properly: it doesn't respect IEEE 754-2008. I prefer to skip the test rather than removing the function on these platforms. What do you think? I tried to compute the sign manually, but oh wow, rules to compute the sign a really complicated :-( By the way, mathmodule.c contains a custom fma() implementation using "TripleLength": |
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Was not it blocked by the fact that the standard fma() implementation on Windows did have unsatisfying quality, so we need to provide our own implementation?
vstinner
commented
Mar 15, 2024
In 2020, test_math failed on Windows 32-bit. It's no longer the case. |
There was a problem hiding this comment.
Looks okay to me.
I prefer to skip the test rather than removing the function on these platforms. What do you think?
Yes, that's a tough one. It makes me really uncomfortable to be offering an fma implementation that we know isn't correct, even on a non-mainstream platform.
Background: math.fma isn't like math.cos or math.exp - the correctness down to the last bit really matters, since that's most of the point of its existence (well, there's the speed aspect too, but we're not going to see the benefits of that at Python level). There are delicate algorithms based on math.fma whose correctness depends on fma giving correctly-rounded results - if it fails to do so, those algorithms can deliver completely wrong results (rather than results that are just off by a few ulps here or there).
So yes, delivering the wrong value on 32-bit Windows was a major blocker.
Delivering the wrong sign of zero on an (at this point) somewhat obscure platform seems like a less heinous crime. In particular, we're at least still getting the right value (as a real number), and I don't know of algorithms that would be thrown by the wrong zero sign.
Can we open upstream issues, so that there's at least a chance that things will eventually be fixed on the underlying platforms? I'm less worried about FreeBSD, and much more interested in WASI, which could yet become mainstream.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Added new math.fma() function, wrapping C99's ``fma()`` operation: fused multiply-add function. Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>
vstinner
commented
Mar 17, 2024
Thanks for the review @mdickinson and @erlend-aasland. I replaced IEEE 754-2008 with IEEE 754. |
vstinner
commented
Mar 18, 2024
I reported the issue to FreeBSD: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277783 |
Added new math.fma() function, wrapping C99's ``fma()`` operation: fused multiply-add function. Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
mhsmith
commented
Mar 25, 2024
This issue also appears on Android x86_64, though ARM64 is OK. And that's not surprising, because Android's libc was partly based on FreeBSD's. I'll add a skip of this test in my next Android PR. |
vstinner
commented
Mar 25, 2024
A fix was proposed for FreeBSD: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277783 If it's merged, maybe propose the same fix to Android libc? Is it bionic?
The best would be to report the issue to Android if you skip the test ;-) |
mhsmith
commented
Mar 25, 2024
Yes, all Android devices use the Bionic libc. I'll watch the FreeBSD issue and maybe report it to Android once it's resolved. |
Added new math.fma() function, wrapping C99's ``fma()`` operation: fused multiply-add function. Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
Added new math.fma() function, wrapping C99's ``fma()`` operation: fused multiply-add function. Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
Added new math.fma() function, wrapping C99's
fma()operation: fused multiply-add function.📚 Documentation preview 📚: https://cpython-previews--116667.org.readthedocs.build/