Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-73468: Add math.fma() function#116667

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 1 commit intopython:mainfromvstinner:fma
Mar 17, 2024
Merged

gh-73468: Add math.fma() function#116667

vstinner merged 1 commit intopython:mainfromvstinner:fma
Mar 17, 2024

Conversation

vstinner
Copy link
Member

@vstinnervstinner commentedMar 12, 2024
edited by github-actionsbot
Loading

Added new math.fma() function, wrapping C99'sfma() operation: fused multiply-add function.


📚 Documentation preview 📚:https://cpython-previews--116667.org.readthedocs.build/

@vstinner
Copy link
MemberAuthor

Previous attempt in 2020:#17987

@vstinnervstinnerforce-pushed thefma branch 2 times, most recently frome988215 to3198308CompareMarch 13, 2024 16:24
@vstinnervstinner added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMar 13, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@vstinner for commit3198308 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMar 13, 2024
@vstinner
Copy link
MemberAuthor

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?

@vstinner
Copy link
MemberAuthor

vstinner commentedMar 13, 2024
edited
Loading

test_math failed on FreeBSD and FreeBSD 14.

test_importlib failed on RHEL7Refleaks, Ubuntu NoGILRefleaks, and MacOS M1Refleaks NoGIL. But it's a known and unrelated leak:#116731

@vstinnervstinner marked this pull request as ready for reviewMarch 13, 2024 20:54
@vstinner
Copy link
MemberAuthor

@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":

/*   The default implementation of dl_mul() depends on the C math library   having an accurate fma() function as required by § 7.12.13.1 of the   C99 standard.    The UNRELIABLE_FMA option is provided as a slower but accurate   alternative for builds where the fma() function is found wanting.   The speed penalty may be modest (17% slower on an Apple M1 Max),   so don't hesitate to enable this build option.       The algorithms are from the T. J. Dekker paper:   A Floating-Point Technique for Extending the Available Precision   https://csclub.uwaterloo.ca/~pbarfuss/dekker1971.pdf*/

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
Copy link
MemberAuthor

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?

In 2020, test_math failed on Windows 32-bit. It's no longer the case.

Copy link
Member

@mdickinsonmdickinson left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 anfma implementation that we know isn't correct, even on a non-mainstream platform.

Background:math.fma isn't likemath.cos ormath.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 onmath.fma whose correctnessdepends onfma 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 yetbecome mainstream.

Added new math.fma() function, wrapping C99's ``fma()`` operation:fused multiply-add function.Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>
@vstinner
Copy link
MemberAuthor

Thanks for the review@mdickinson and@erlend-aasland. I replaced IEEE 754-2008 with IEEE 754.

@vstinnervstinner merged commit8e3c953 intopython:mainMar 17, 2024
@vstinnervstinner deleted the fma branchMarch 17, 2024 13:58
@vstinner
Copy link
MemberAuthor

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.

I reported the issue to FreeBSD:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277783

vstinner added a commit to vstinner/cpython that referenced this pull requestMar 20, 2024
Added new math.fma() function, wrapping C99's ``fma()`` operation:fused multiply-add function.Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
@mhsmith
Copy link
Member

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
Copy link
MemberAuthor

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.

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?

I'll add a skip of this test in my next Android PR.

The best would be to report the issue to Android if you skip the test ;-)

@mhsmith
Copy link
Member

Yes, all Android devices use the Bionic libc. I'll watch the FreeBSD issue and maybe report it to Android once it's resolved.

vstinner reacted with heart emoji

adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
Added new math.fma() function, wrapping C99's ``fma()`` operation:fused multiply-add function.Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
Added new math.fma() function, wrapping C99's ``fma()`` operation:fused multiply-add function.Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mdickinsonmdickinsonmdickinson approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@tim-onetim-oneAwaiting requested review from tim-one

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@vstinner@bedevere-bot@mhsmith@mdickinson@serhiy-storchaka@erlend-aasland

[8]ページ先頭

©2009-2025 Movatter.jp