Skip to content

Make NaNs quiet by default, and make signalling NaNs distinct#10432

Closed
LewisGaul wants to merge 3 commits intoziglang:masterfrom
LewisGaul:LG-qnan
Closed

Make NaNs quiet by default, and make signalling NaNs distinct#10432
LewisGaul wants to merge 3 commits intoziglang:masterfrom
LewisGaul:LG-qnan

Conversation

@LewisGaul
Copy link
Copy Markdown
Contributor

This allows the std.math functions to return the same NaN that matches GCC/Musl for non-NaN inputs (related: #10415).

Note the implementation of the nan() and snan() functions could be made generic if/when #10133 is merged, which would be something that could be addressed in this PR...

@LewisGaul
Copy link
Copy Markdown
Contributor Author

The failing math.isSignalNan testcases seems to be because Zig converts f32 signalling NaNs to quiet NaNs when they're passed into a function (yes, seems specific to f32), i.e. sets the first bit of the mantissa. Tracked in #10449. I will comment out that part of the testcase for now.

@tiehuis
Copy link
Copy Markdown
Member

tiehuis commented Jan 2, 2022

I generally agree with cleaning up nan handling and defaulting to quiet nans but a few comments about this at a high-level.

This has some good reading on an overview of how signalling/quiet nans are differentiated in C.
https://stackoverflow.com/a/55648118/9465671

The key takeaways are:

  • A signalling nan should raise an fpu exception when an operation is performed which utilizes the signalling nan
  • The bit representation is platform specific as to what is signalling or not and is not consistent between platforms. This was standardized in IEEE754-2008 but not required so old platforms were still compliant.

All binary NaN bit strings have all the bits of the biased exponent field E set to 1 (see 3.4). A quiet NaN bit
string should be encoded with the first bit (d1) of the trailing significand field T being 1. A signaling NaN
bit string should be encoded with the first bit of the trailing significand field being 0. If the first bit of the
trailing significand field is 0, some other bit of the trailing significand field must be non-zero to distinguish
the NaN from infinity. In the preferred encoding just described, a signaling NaN shall be quieted by setting
d1 to 1, leaving the remaining bits of T unchanged.

I would probably go the route of removing explicit signalling nan support (isSignallingNan and snan constants) and only explicitly supporting quiet nans.

Quiet nan constants we emit should then be based on the above as mentioned in the standard (e.g. 32-bit repr should be 0x7fc0000). I see that you are matching musl/glibc here so I may need a closer look at this.

Perhaps we can add a function similar to nanf to generate nans with arbitrary mantissa values as well while we are at it?

@LewisGaul
Copy link
Copy Markdown
Contributor Author

This has some good reading on an overview of how signalling/quiet nans are differentiated in C. https://stackoverflow.com/a/55648118/9465671

Thanks, that helped my understanding of NaNs!

  • A signalling nan should raise an fpu exception when an operation is performed which utilizes the signalling nan

I understand that after reading the above. My understanding is that FP exceptions aren't properly supported in Zig yet, and I'd consider worrying about this to be out of the scope of this PR, would you agree?

  • The bit representation is platform specific as to what is signalling or not and is not consistent between platforms. This was standardized in IEEE754-2008 but not required so old platforms were still compliant.

Ah ok... Is this something we need to worry about, and if so what would you suggest doing about it?

I would probably go the route of removing explicit signalling nan support (isSignallingNan and snan constants) and only explicitly supporting quiet nans.

I'm happy to go this route. I guess the point you're making is that my implementation of isSignallingNan(), while conformant with IEEE-754, may not be correct on all platforms, so better to remove it (or begin deprecating it)?

Quiet nan constants we emit should then be based on the above as mentioned in the standard (e.g. 32-bit repr should be 0x7fc0000). I see that you are matching musl/glibc here so I may need a closer look at this.

This is what musl/glibc do, so yes this PR changes quiet NaNs to be IEEE-754 conformant.

Perhaps we can add a function similar to nanf to generate nans with arbitrary mantissa values as well while we are at it?

I'd be happy to add that, if it's something we want.

@tiehuis
Copy link
Copy Markdown
Member

tiehuis commented Jan 2, 2022

I understand that after reading the above. My understanding is that FP exceptions aren't properly supported in Zig yet, and I'd consider worrying about this to be out of the scope of this PR, would you agree?

This is correct. This would require something like https://en.cppreference.com/w/c/numeric/fenv in general and ties in a bit to non-local program state to control fpu registers since by default I know some platforms don't have things like nan exceptions enabled.

Ah ok... Is this something we need to worry about, and if so what would you suggest doing about it?

We could probably push nan generation for any bit size behind a function for now and use the existing bit representation. I am not sure what this would look like for other platforms right now.

I'm happy to go this route. I guess the point you're making is that my implementation of isSignallingNan(), while conformant with IEEE-754, may not be correct on all platforms, so better to remove it (or begin deprecating it)?

Correct. Along with removing the snan constants since as we don't actually provide a useful mechanism right now for managing this to a user, there would be a manual process anyway; so let's not give them something which they may assume work (raises fpu exceptions). We would just provide a "nan" which is always quiet. I would be interested to see some use cases of signalling nan in practice as I personally don't used.

Right now if a user really needs signalling nan support they would need to use fenv from c anyway, in which case they can use some of the relevant c functions for managing imo, until we decide what this looks like in zig.

@andrewrk
Copy link
Copy Markdown
Member

@tiehuis thanks for working with @LewisGaul on this! Looking forward to seeing what you two come up with. I'm closing this PR because it's old, it has conflicts, and the CI was never passing, but of course please open a fresh one if you want to push forward.

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.

3 participants