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

Replace use of FLT_MAX in x86_64 zscal.c by isinf() #4717

Merged
merged 1 commit into from
May 26, 2024

Conversation

bartoldeman
Copy link
Contributor

Commit def4996 fixed issues with inf and nan values in zscal, but used FLT_MAX, where DBL_MAX or isinf() is more appropriate, as FLT_MAX is for single precision only.
Using FLT_MAX caused test case failures in the LAPACK tests.

isinf() is consistent with the later fix 969601a

Commit def4996 fixed issues with inf and nan values in zscal,
but used FLT_MAX, where DBL_MAX or isinf() is more appropriate,
as FLT_MAX is for single precision only.
Using FLT_MAX caused test case failures in the LAPACK tests.

isinf() is consistent with the later fix 969601a
@bartoldeman
Copy link
Contributor Author

bartoldeman commented May 24, 2024

This fixes the "other error" failures showed here:
#4625 (comment)

@martin-frbg
Copy link
Collaborator

I think it was the question of portability that kept me from using isinf() originally - not switching between FLT_MAX and DBL_MAX for cscal/zscal was sloppy indeed.

@bartoldeman
Copy link
Contributor Author

Yes isinf() requires C99, and wasn't in MSVC for a long time but since 2013 it's even there. You'll have a hard time finding a current compiler that doesn't support it unless I missed something. There is a workaround: x == x && (x - x) != (x - x).

Note that here is just a minimal fix, and cscal is still affected by the original issue. I would just remove the special cases for real and purely imaginary alpha, since they are both incorrect for nan (1*(nan+0i)==nan+nani, as weird as it is).
Of course there's a performance penalty, but the checks we have now in zscal may be more expensive than just doing the full complex multiplication! And for real alpha there's always dzscal (but if I understand the code correctly, dzscal simply calls zscal kernels). An initial change would be to then simply remove the purely imaginary special code paths.

In
https://people.eecs.berkeley.edu/~demmel/Exception_Handling_for_the_BLAS_and_LAPACK_12Aug2021.pdf
I think the general gist is to make an exception for alpha=0 (or beta=0 in dgemm etc), but propagate NaNs everywhere else.

@martin-frbg
Copy link
Collaborator

I wonder if there are still folks stuck with ancient "enterprise" distributions installed on their clusters... and there's more fun with zscal, oneapi's icx appears to think that nan can be equal to 1 :(

@martin-frbg martin-frbg added this to the 0.3.28 milestone May 26, 2024
@martin-frbg martin-frbg merged commit 7721168 into OpenMathLib:develop May 26, 2024
72 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants