MAINT: Fix floating point warning flag as much as possible #19476
Conversation
|
Well, that attempt died quickly on CI :/, it had worked great locally. (I still have some hope I just missed some SIMD path though) |
|
Aha, I managed to rewrite the AVX512F logic to avoid the spurious warning setting. Quick timing gives (on a machine with On main: On this branch: So it looks like <5% slowdown, probably we can live with that. I think the AVX512 solution may be a big improvement for the SSE2 code. That code currently tests for an invalid flag set during the comparison. But as far as I can tell, both of the instructions I now use in AVX have equivalent ones in SSE2. |
This is technically even incorrect in IEEE, signalling NaNs should pretty much always give a warning. But overall, it seems unlikely NumPy should bother about it (we never create them after all). This is split off from numpygh-19476. Mainly, to check whether s390x is broken...
fd815bd to
7291c4b
Compare
|
I can reproduce the MacOS failures locally with clang, and they go away with So we will have to figure out gh-19049 before this has any chance :(. Marking as draft, I guess some of the SIMD stuff could be split out regardless of clang. EDIT: xref the issue gh-19427 |
7291c4b to
f137362
Compare
|
Well, probably will have to just pull out the useful parts... Godbolt points out that I am not actually sure why this fails CI, since that looks like a clang 11, unless it is Mac specific? But, I guess we support clang 9 anyway, so... EDIT: Apparently the CI issue is that clang 11 does not support trapping-math, at least not for x86_64. |
|
Closing reopening, because I am curious if bumping MacOS in CI from 10.14 to 10.15 makes a difference here (although I am not sure that would actually help with moving forward). |
This PR avoids setting FPE flag flags rather than clearing them where possible. The main place where this is not easy is SSE (I could not yet find the `isless` equivalent for basic SSE), and did not want to modify the code too much. This also updates the docs to copy the informational section to the "Floating point error handling" landing page. Right now, the docs assume that we (for now) keep the behaviour of NOT giving floating point warnings for comparisons with NaN. Hopefully, using these functions should not have any speed impact. There is some chance that compilers will not honor the contract and replace `isless` with instructions that do set floating point error flags. If/where this happens, we may have to undo the changes. This should have no impact on functionality (except if compilers do not follow C99/IEEE correctly, which is entirely possible). However, signalling NaNs WILL now warn more often (as per C99/IEEE), since we do not clear the warnings as agressively.
f137362 to
1110d7c
Compare
|
Going to close this for now. There are a few things here that would be nice to have. The whole thing would be nice, but seems only viable for newer clang versions (which probably takes a bit more time). |
This PR avoids setting FPE flag flags rather than clearing them
where possible.
The main place where this is not easy is SSE (I could not yet find
the
islessequivalent for basic SSE), and did not want to modifythe code too much.
This also updates the docs to copy the informational section to the
"Floating point error handling" landing page. Right now, the docs
assume that we (for now) keep the behaviour of NOT giving floating
point warnings for comparisons with NaN.
Hopefully, using these functions should not have any speed impact.
There is some chance that compilers will not honor the contract
and replace
islesswith instructions that do set floating pointerror flags. If/where this happens, we may have to undo the changes.
This should have no impact on functionality (except if compilers
do not follow C99/IEEE correctly, which is entirely possible).
However, signalling NaNs WILL now warn more often (as per C99/IEEE),
since we do not clear the warnings as agressively.
A few notes:
islessand friends won't work perfectly. There is also some chance of performance regressions forcing us back.To some degree getting warning flags right seems a bit "aspirational"...