Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Don't signal FP exceptions in np.absolute #8713

Merged
merged 1 commit into from Mar 7, 2017

Conversation

jcowgill
Copy link
Contributor

Fixes #8686

This PR centers around this piece of code in numpy/core/src/umath/loops.c.src:

UNARY_LOOP {
    const @type@ in1 = *(@type@ *)ip1;
    const @type@ tmp = in1 > 0 ? in1 : -in1;
    /* add 0 to clear -0.0 */
    *((@type@ *)op1) = tmp + 0;
}

If in1 is NaN, the C99 standard requires that the comparison in1 > 0 signals FE_INVALID, but the usual semantics for the absolute function are that no FP exceptions should be generated (eg compare to C fabs and Python abs). This was probably never noticed due to a bug in GCC x86 where all floating point comparisons do not signal exceptions, however Clang on x86 and GCC on other architectures (including ARM and MIPS) do signal an FP exception here.

Fix by rewriting the loop to use npy_fabs instead. Also fix a similar occurrence in simd.inc.src. The test_abs_neg_blocked is adjusted not to ignore FE_INVALID errors because now both absolute and negate should never produce any FP exceptions.

@juliantaylor
Copy link
Contributor

This would have significant performance impact on non x86 as the function is not inlined.
Probably better to just use npy_clear_floatstatus at the end

@juliantaylor
Copy link
Contributor

the change makes sense, but we compile with clang and windows compilers, how come we never saw this outside of mips?

@jcowgill
Copy link
Contributor Author

This would have significant performance impact on non x86 as the function is not inlined.
Probably better to just use npy_clear_floatstatus at the end

Ah, I didn't check that - I was just testing with plan fabs which GCC does inline. It's frustrating because using fabs on MIPS is about 5x faster than the original code since fabs can be implemented with just one AND instruction.

the change makes sense, but we compile with clang and windows compilers, how come we never saw this outside of mips?

I can reproduce the bug on Clang x86. The test suite was ignoring FP exceptions before which may be why noone saw it.

Fixes numpy#8686

This PR centers around this piece of code in `numpy/core/src/umath/loops.c.src`:
```c
UNARY_LOOP {
    const @type@ in1 = *(@type@ *)ip1;
    const @type@ tmp = in1 > 0 ? in1 : -in1;
    /* add 0 to clear -0.0 */
    *((@type@ *)op1) = tmp + 0;
}
```

If in1 is `NaN`, the C99 standard requires that the comparison `in1 > 0`
signals `FE_INVALID`, but the usual semantics for the absolute function are
that no FP exceptions should be generated (eg compare to C `fabs` and Python
`abs`). This was probably never noticed due to a bug in GCC x86 where all
floating point comparisons do not signal exceptions, however Clang on x86 and
GCC on other architectures (including ARM and MIPS) do signal an FP exception
here.

Fix by clearing the floating point exceptions after the loop has
finished. The alternative of rewriting the loop to use `npy_fabs`
instead would also work but has performance issues because that function
is not inlined. The `test_abs_neg_blocked` is adjusted not to ignore
`FE_INVALID` errors because now both absolute and negate should never
produce an FP exceptions.
@jcowgill
Copy link
Contributor Author

jcowgill commented Mar 7, 2017

Sorry for the delay - I've updated the PR to use npy_clear_floatstatus.

@juliantaylor
Copy link
Contributor

thanks

@juliantaylor juliantaylor merged commit 2fe5a47 into numpy:master Mar 7, 2017
@juliantaylor
Copy link
Contributor

@charris while I haven't seen it myself it appears to cause warnings with clang, so it might be worthwhile to still put this into 1.12.1, should be regression safe.

@juliantaylor
Copy link
Contributor

math functions are now inlined (gh-8754) so one could now use npy_fabs for better performance on some arches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

np.abs(np.nan) causes floating point exception on MIPS
3 participants