ENH, BLD: Fix math feature detection for wasm #21154
Conversation
|
It seems like If this is int exp (void); // some incorrect signature for `exp`
int main (void) {
exp();
return 0;
}and double exp (double); // the correct signature for `exp`
int main (void) {
exp();
return 0;
}then Out of MANDATORY_FUNCS = ["sin", "cos", "tan", "sinh", "cosh", "tanh", "fabs",
"floor", "ceil", "sqrt", "log10", "log", "exp", "asin",
"acos", "atan", "fmod", 'modf', 'frexp', 'ldexp']if the wrong signatures are given, every single function requires |
|
On Mac, dynamic linking is failing with "Symbol not found: _npy_pow" I think the windows second failure suggests that on windows:
|
The web assembly linker is strict about function types, it is unwilling to link a function defined with signature say `double f(double, double)` to an invocation with signature `void f(void)`. This causes trouble in `numpy/core/setup.py` in the functions `check_math_capabilities` and `check_mathlib`. This patch fixes the problem by giving config.try_link the correct function signatures for these functions. In particular I added a separate header file with all the math declarations. It would be be possible to just include 'math.h' but we also need to parse the declarations to figure out how to call the functions. If f has arguments type1, type2, type3, we want to call it like f((type1)0, (type2)0, (type3)0). Anyways it is easier to parse the arguments out of our feature_detection_math.h than to worry about the possibility that someone will have a differently formatted system math.h. We do a test where we include both math.h and feature_detection_math.h to ensure consistency between the signatures. I also separated out the fcntl functions, backtrace and madvise, and strtold_l. This is because they require separate headers. strtold_l requires testing both with the locale.h header and with the xlocale.h header (this seems to vary even among different linuxes). All of the functions I moved out of OPTIONAL_STDFUNCS are absent on windows and some are absent on OSX so separating them out of the OPTIONAL_STDFUNCS should mildly improve the speed of feature detection on mac and windows (since they have all the functions remaining in OPTIONAL_STDFUNCS).
7da3b7a to
3e4d87b
Compare
Not really. Does it depend on compiler/version of compiler? I find the Compiler Explorer https://godbolt.org/ can be useful for things like this. |
Our package build process currently has a significant flaw: we first run setup.py, recording all compilation commands, then we rewrite these compilation commands to invoke emcc and replay them, and then we pray that the cross compiled executables ended up in the right place to go into the wheel. This is not a good strategy because the build script is allowed to implement arbitrary logic, and if it moves, renames, etc any of the output files then we lose track of them. This has repeatedly caused difficulty for us. However, we also make no particularly significant use of the two pass approach. We can just do the simpler thing: capture the compiler commands as they occur, modify them as needed, and then run the fixed command. I also added a patch to fix the numpy feature detection for wasm so that we don't have to include _npyconfig.h and config.h, numpy can generate them in the way it would for a native build. I opened a numpy PR that would fix the detection for us upstream: numpy/numpy#21154 This clears the way for us to switch to using pypa/build (as @henryiii has suggested) by removing our dependence on specific setuptools behavior. This is on top of #2238.
|
That old code hurts a little to look at. Changes are more verbose, but I don't see a problem merging them to improve WASM support - it won't really make build system migration more difficult. A few other thoughts since I didn't look at this code in ages:
|
Incidentally, I was also looking for |
|
@h-vetinari while there are some dummy fallback definitions (using double power), those are just aliases for the C power functions (they are C99, so they at least should always exist). I bet we don't need them. However, the ufunc, operator, and scalars may have their own paths (especially for integer powers) and to define fast-paths for certain powers. |
That's actually what I was trying to find out. I wanted to know how |
|
@h-vetinari there are some special paths for the operator numpy/numpy/core/src/multiarray/number.c Line 449 in 7653829 The ufunc is "generated": That may well be a bad idea: It will prevent any inlining of the pow function (or further Compiler optimizations), and just call the math libraries pow. It may well be that this did not matter at the time it was written, but now would be much faster to write explicitly. (The code does not generate a loop that would allow inlining, maybe that would be a solution as well: Change how the code generator works for f=...).
That is... If it is worthwhile. |
|
Would appreciate if someone could review this @rgommers. We have merged exactly this patch into Pyodide |
mattip
left a comment
There was a problem hiding this comment.
LGTM. It always bothered me that we cannot build with -Wall -Werror.
|
Could you add a note in |
|
@charris How's this look? |
|
Bumping this. |
|
here is the rendered note. |
|
Flaky test_vector_matrix_values on win32 failure not connected to this PR. Merging |
|
Thanks @hoodmane |
|
xref #21584 where these changes seem to cause some failure during detection of the required math functions (cos, etc.) on https://termux.com/. |
|
I just read this article which suggests feature detecting as follows: int main()
{
double (*p)(const char *, char **, locale_t) = strtod_l;
}Though this won't work if some toolchains define the function as a function-like macro. |
The article seems to say that that's a feature - but it seems to me it's a problem, right? |
|
To be fair I think neither the approach prior to this pr nor the one in this pr detect function-like macros. You would need to include the real headers for that. But I don't think the linked article is very clearly written -- it doesn't have any examples so it's hard to know in what edge cases these approaches behave differently. |
Our package build process currently has a significant flaw: we first run setup.py, recording all compilation commands, then we rewrite these compilation commands to invoke emcc and replay them, and then we pray that the cross compiled executables ended up in the right place to go into the wheel. This is not a good strategy because the build script is allowed to implement arbitrary logic, and if it moves, renames, etc any of the output files then we lose track of them. This has repeatedly caused difficulty for us. However, we also make no particularly significant use of the two pass approach. We can just do the simpler thing: capture the compiler commands as they occur, modify them as needed, and then run the fixed command. I also added a patch to fix the numpy feature detection for wasm so that we don't have to include _npyconfig.h and config.h, numpy can generate them in the way it would for a native build. I opened a numpy PR that would fix the detection for us upstream: numpy/numpy#21154 This clears the way for us to switch to using pypa/build (as @henryiii has suggested) by removing our dependence on specific setuptools behavior. This is on top of #2238.
The web assembly linker is strict about function types, it is
unwilling to link a function defined with signature say double
f(double, double) to an invocation with signature void f(void). This
causes trouble in numpy/core/setup.py in the functions
check_math_capabilities and check_mathlib.
This patch fixes the problem by giving config.try_link the correct
function signatures for these functions. In particular I added a
separate header file with all the math declarations. It would be be
possible to just include 'math.h' but we also need to parse the
declarations to figure out how to call the functions. If f has
arguments type1, type2, type3, we want to call it like f((type1)0,
(type2)0, (type3)0). Anyways it is easier to parse the arguments out
of our feature_detection_math.h than to worry about the possibility
that someone will have a differently formatted system math.h. We do a
test where we include both math.h and feature_detection_math.h to
ensure consistency between the signatures.
I also separated out the fcntl functions, backtrace and madvise, and
strtold_l. This is because they require separate headers. strtold_l
requires testing both with the locale.h header and with the xlocale.h
header (this seems to vary even among different linuxes). All of the
functions I moved out of OPTIONAL_STDFUNCS are absent on windows and
some are absent on OSX so separating them out of the
OPTIONAL_STDFUNCS should mildly improve the speed of feature
detection on mac and windows (since they have all the functions
remaining in OPTIONAL_STDFUNCS).