MAINT: Simplify npymath #22090
MAINT: Simplify npymath #22090
Conversation
|
The result of #9567 from 2017 was to blocklist hypot on 32-bit windows. When I rerun the test file after compiling with cl.exe for x86 from VS2019, I get that the floating point flags are not changed: Maybe in the mean time Microsoft fixed the problem? We do have a |
|
Thanks Matti, this looks like a pretty good start already. Some of the outdated long double formats also stood out to me, but probably only worth touching if they're a pain for moving build systems; otherwise best to wait and deprecate it all in one go later. |
|
If we wish to vendor the npmath sources into the wheel, this will require thinking about how to package the build-time generated |
That can't work I think, the whole point of this header is to account for platform/OS/compiler-specific differences, so we can't just ship one |
|
cygwin still needs a blocklisted |
|
rabbit hole: Edit: looking again at what I did here, I removed the alternative npy_powl which caused the CI failure. I will leave #8608 for a future enhancement. |
0300760 to
604e453
Compare
|
I think I have gone about as far as I can go until we can drop manylinux2014 (in 2024), msvc 2015 (in 2025?) and 32-bit mingw (which may have fixed hypot and atan2, but we don't test it). Maybe by then cygwin will have fixed its complex functions as well. |
|
I agree with @h-vetinari - let's drop VS < 2019 . Does anyone have a good argument not to? |
|
The overflow warning in the complex |
|
Note that any pruning of symbols used in |
This PR should not remove any of the exported functions in |
|
I have started on the Meson build of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready for review. npy_math_internal.h.src is the most complicated change. Most of what it does is to define functions like
float npy_sinf(float x) {
return sinf(x);
}
A second pass through npymath could define macros for many of these mandatory functions rather than generating functions for them
#define npy_sin sin
#define npy_sinf sinf
#define npy_sinl sinl
Some "optional" functions have more correct implementations. These are the same, with a small refactoring tweak.
| @@ -156,7 +157,7 @@ def check_funcs_once(funcs_name, headers=["feature_detection_math.h"]): | |||
| call_args=call_args, | |||
| headers=headers, | |||
| ) | |||
| if st: | |||
| if st and add_to_moredefs: | |||
| moredefs.extend([(fname2def(f), 1) for f in funcs_name]) | |||
There was a problem hiding this comment.
If add_to_moredefs=False then the function will not result in a HAVE_FUNC macro in config.h. There is no reason for the mandatory C99 functions to have that macro, if they are missing the NumPy build will fail during setup. I assume downstream users are not consuming these macros.
| #include "numpy/numpyconfig.h" | ||
| #include "numpy/npy_cpu.h" | ||
| #include "numpy/utils.h" |
There was a problem hiding this comment.
The removed headers are needed for simd dispatch, and are not part of the configuring done here.
|
Needs a merge conflict resolved after gh-22159 was merged. |
39f654a to
d1cbf3f
Compare
| #else | ||
| /* ok on 64 bit posix */ | ||
| return PyOS_strtol(str, endptr, base); | ||
| #endif | ||
| } | ||
|
|
||
| NPY_NO_EXPORT npy_ulonglong | ||
| NumPyOS_strtoull(const char *str, char **endptr, int base) |
There was a problem hiding this comment.
Should these now more or less pointless wrapper functions be deprecated somehow...?
There was a problem hiding this comment.
Agreed there is more cleanup that can be done to turn one-line functions into defines (if they are exported) or to deprecate them (if they are internal). I was primarily targeting cleaning up the dead code from npymath in this PR. Maybe we should have a github project for code cleanup and add this as a task.
|
I think this is a complete unit. There are at least two continuations:
|
May make sense, not entirely clear given that |
|
I just browsed through and the changes look great. I think I remember seeing that AIX might not define |
|
Last changes look good, I do want to do a bit of testing on macOS arm64 and verify no symbols went missing. |
|
On a macmini running |
|
I'm not seeing that. Running
I recommend not doing this, it is not necessary and running the non-native architecture like that has given me problems since the PowerPC OSX days. |
|
In my m1 arm64 build using the system-provided compiler, numpy/numpy/core/include/numpy/numpyconfig.h Lines 26 to 28 in acdd0e3 NPY_SIZEOF_LONGDOUBLE should be 16. I wonder why you do not hit that in your build. Do you see the __APPLE__ and __arm64__ macros if you dotouch /tmp/foo.c; clang -dM -E foo.c |grep "\(APPLE\)\|\(arm64\)"?
|
|
Yes: % touch /tmp/foo.c; clang -dM -E /tmp/foo.c |grep "\(APPLE\)\|\(arm64\)"
#define __APPLE_CC__ 6000
#define __APPLE__ 1
#define __arm64 1
#define __arm64__ 1EDIT: I have also verified that I get the same result with Apple Clang and conda-forge provided Clang.
I don't think so - |
|
This is a giveaway that you are not actually running the |
|
Tested on Linux as well (including rebuilding SciPy and ensuring |
I believe the spec is 16 on aarch64, but Apple broke the spec for M1 and made it the same as double. |
Towards solving #20880. So far this removes support for outdated Solaris, rearranges the headers a bit, adds some double C99 routines to mandatory math functions, removes emitting HAVE_* macros for mandatory functions, and removes code paths in the cases of missing mandatory functions.
It seems we cannot remove the complex math function implementations until we drop manylinux2014 (glibc 2.17) which does not go EOL until June 2024.