-
Notifications
You must be signed in to change notification settings - Fork 122
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
flamenco/tests: fix double normal classification #1848
base: main
Are you sure you want to change the base?
Conversation
@@ -97,7 +97,7 @@ static int | |||
fd_double_is_normal( double dbl ) { | |||
ulong x = fd_dblbits( dbl ); | |||
int is_denorm = | |||
( fd_dblbits_bexp( x ) == 0 ) | | |||
( fd_dblbits_bexp( x ) == 0 ) & | |||
( fd_dblbits_mant( x ) != 0 ); |
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.
This function seems like it would mistakenly classify 0 as a normal number. Could you please double check that? If so then this line can be removed, or we should add another case that explicitly checks for 0 (for neatness)
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.
@anarcheuz it would be good to add a comment here explaining that we are explicitly allowing zero, and the reason.
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.
Based on my reading of what I find on the Internet, 0 is not a subnormal number
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.
@ptaffet-jump That is true, but the C/C++ definition of "isnormal" is "not a NaN, not infinity, not subnormal, and not zero". It was my mistake to not put the "is_zero" check into fd_double_is_normal.
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.
Looks good, could we not merge this until Agave also accepts 0?
@@ -97,7 +97,7 @@ static int | |||
fd_double_is_normal( double dbl ) { | |||
ulong x = fd_dblbits( dbl ); | |||
int is_denorm = | |||
( fd_dblbits_bexp( x ) == 0 ) | | |||
( fd_dblbits_bexp( x ) == 0 ) & | |||
( fd_dblbits_mant( x ) != 0 ); |
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.
@anarcheuz it would be good to add a comment here explaining that we are explicitly allowing zero, and the reason.
Would you also mind changing the commit message? How about |
36ad4f4
to
ab20f9e
Compare
Done. |
@mjain-jump I think you mentioned that Agave accepts 0 (ie. not causing mismatch)? Other than that, |
Agave indeed seems to be accepting 0. |
Tested by differential fuzzing with C++'s
std::isnormal()
. Without the fix we would reject normal double such as0x93da8e
that comes from the fuzzer (ie. lower coverage) whereasstd::normal()
would accept it.-ffast-math
prevents us from usingstd::isnormal()
.Note: We are purposely not rejecting 0, because it is an interesting value from a fuzzing perspective. With @ripatel-fd we suggest to change the harnesses to accept 0 as a valid fuzzing value.