Skip to content

ENH, BLD: Fix math feature detection for wasm #21154

Merged
mattip merged 3 commits intonumpy:mainfrom
hoodmane:feature-detection-sigs
Mar 31, 2022
Merged

ENH, BLD: Fix math feature detection for wasm #21154
mattip merged 3 commits intonumpy:mainfrom
hoodmane:feature-detection-sigs

Conversation

@hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Mar 6, 2022

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).

@charris charris added 03 - Maintenance 09 - Backport-Candidate PRs tagged should be backported labels Mar 6, 2022
@hoodmane
Copy link
Contributor Author

hoodmane commented Mar 6, 2022

It seems like check_mathlib actually depends on the signature for exp being wrong to work correctly? Very strange.

If this is testa.c:

int exp (void); // some incorrect signature for `exp`
int main (void) {
  exp();
  return 0;
}

and testb.c:

double exp (double); // the correct signature for `exp`
int main (void) {
  exp();
  return 0;
}

then cc -c testa.c -o testa.o followed by cc testa.o -o testa fails, but if you link with -lm the linking works. On the other hand, when declared with the correct signature, it will link even without -lm.

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 -lm to link. However, with correct signatures, only log and fmod do. Does anyone understand this?!

@charris charris changed the title FIX numpy/core/setup.py math feature detection on wasm targets MAINT: Fix numpy/core/setup.py math feature detection for wasm Mar 6, 2022
@charris charris changed the title MAINT: Fix numpy/core/setup.py math feature detection for wasm MAINT, BLD: Fix math feature detection for wasm Mar 6, 2022
@hoodmane
Copy link
Contributor Author

hoodmane commented Mar 6, 2022

On Mac, dynamic linking is failing with "Symbol not found: _npy_pow"
Cygwin also seems to be failing due to missing npy_pow symbol
On windows, TestSpecialFloats.test_arcsinh and TestSpecialFloats.test_arccosh fail, they expected nan but get +/-inf.

I think the windows second failure suggests that on windows:

  1. for some reason my modification makes it detect that asinh and acosh aren't available
  2. the fallback implementations of acosh and asinh handle nan slightly differently than the system math library implementations.

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).
@hoodmane hoodmane force-pushed the feature-detection-sigs branch 4 times, most recently from 7da3b7a to 3e4d87b Compare March 7, 2022 23:54
@mattip
Copy link
Member

mattip commented Mar 8, 2022

Does anyone understand this?!

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.

hoodmane added a commit to hoodmane/pyodide that referenced this pull request Mar 13, 2022
hoodmane added a commit to pyodide/pyodide that referenced this pull request Mar 13, 2022

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.
@rgommers
Copy link
Member

rgommers commented Mar 15, 2022

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:

@h-vetinari
Copy link
Contributor

On Mac, dynamic linking is failing with "Symbol not found: _npy_pow"
Cygwin also seems to be failing due to missing npy_pow symbol

Incidentally, I was also looking for npy_pow today, and couldn't find the source (commenting here because this is the only issue/PR that shows up by searching for it). I don't have full confidence that the GH code search finds all instances, but also locally I didn't get much more:

> findstr /L /S /N npy_pow numpy/*.*
numpy/core/include/numpy/npy_math.h:201:NPY_INPLACE double npy_pow(double x, double y);
numpy/core/include/numpy/npy_math.h:305:NPY_INPLACE float npy_powf(float x, float y);
numpy/core/include/numpy/npy_math.h:348:NPY_INPLACE npy_longdouble npy_powl(npy_longdouble x, npy_longdouble y);
numpy/core/src/npymath/npy_math_complex.c.src:422:/* private function for use in npy_pow{f, ,l} */
numpy/core/src/npymath/npy_math_internal.h.src:569:        return -npy_pow@c@(-x, 1. / 3.);
numpy/core/src/npymath/npy_math_internal.h.src:572:        return npy_pow@c@(x, 1. / 3.);
numpy/core/src/umath/scalarmath.c.src:419:    *out = npy_pow@c@(a, b);
numpy/core/src/umath/scalarmath.c.src:428:    const npy_float outf = npy_powf(af,bf);

@seberg
Copy link
Member

seberg commented Mar 19, 2022

@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.

@h-vetinari
Copy link
Contributor

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 power is implemented to try to determine whether it would be more beneficial performance-wise to translate power(2, x) to exp(log(2) * x) (even though it's likely implemented like that...).

@seberg
Copy link
Member

seberg commented Mar 21, 2022

@h-vetinari there are some special paths for the operator **, trying the following:

fast_scalar_power(PyObject *o1, PyObject *o2, int inplace,

The ufunc is "generated":

TD(inexact, f='pow', astype={'e': 'f'}),

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.

@hoodmane
Copy link
Contributor Author

Would appreciate if someone could review this @rgommers. We have merged exactly this patch into Pyodide
pyodide/pyodide#2256
so in addition to passing the CI here, it also works correctly in wasm as I asserted (previously we had a hack to disable the feature detection and hard code the results).

Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

LGTM. It always bothered me that we cannot build with -Wall -Werror.

@charris charris changed the title MAINT, BLD: Fix math feature detection for wasm ENH, BLD: Fix math feature detection for wasm Mar 26, 2022
@charris
Copy link
Member

charris commented Mar 26, 2022

Could you add a note in doc/release/upcoming_changes? Look at other notes to see how that is done.

@hoodmane
Copy link
Contributor Author

@charris How's this look?

@hoodmane
Copy link
Contributor Author

Bumping this.

@mattip
Copy link
Member

mattip commented Mar 31, 2022

here is the rendered note.

@mattip
Copy link
Member

mattip commented Mar 31, 2022

Flaky test_vector_matrix_values on win32 failure not connected to this PR. Merging

@mattip mattip merged commit 3f9fada into numpy:main Mar 31, 2022
@mattip
Copy link
Member

mattip commented Mar 31, 2022

Thanks @hoodmane

@hoodmane hoodmane deleted the feature-detection-sigs branch March 31, 2022 16:21
@hoodmane
Copy link
Contributor Author

Thanks @mattip (and @charris, @rgommers)!

@charris charris added 36 - Build Build related PR and removed component: build 09 - Backport-Candidate PRs tagged should be backported labels Mar 31, 2022
@seberg
Copy link
Member

seberg commented May 24, 2022

xref #21584 where these changes seem to cause some failure during detection of the required math functions (cos, etc.) on https://termux.com/.

@hoodmane
Copy link
Contributor Author

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.

@rgommers
Copy link
Member

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?

@hoodmane
Copy link
Contributor Author

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.

ryanking13 pushed a commit to pyodide/pyodide-build that referenced this pull request Jun 10, 2024

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants