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

Make sNaN removal code tolerate different sNaN encodings #43025

Merged
merged 1 commit into from
Jul 4, 2017

Conversation

est31
Copy link
Member

@est31 est31 commented Jul 3, 2017

IEEE 754-1985 specifies the encoding of NaN floating point numbers,
but while it mentions that NaNs can be subdivided into signaling
and quiet ones, it doesn't fix the encoding of signaling NaNs in binary
formats. This led to different implementations (CPUs) having different
encodings. IEEE 754-2008 finally specified the encoding of signaling NaNs
but some architectures are compatible with it, while others aren't.
Certain MIPS and PA-RISC CPUs have different encodings for signaling
NaNs.

In order to have the float <-> binary cast feature of the std library be
portable to them, we don't mask any quiet NaNs like we did before (only
being compliant to IEEE 754-2008 and nothing else), but instead we
simply pass a known good NaN instead.

Note that in the code removed there was a bug; the 64 bit mask for quiet
NaNs should have been 0x0008000000000000 instead of the specified
0x0001000000000000.

@est31
Copy link
Member Author

est31 commented Jul 3, 2017

Hopefully, with this PR merged, the feature can be stabilized for Rust 1.20.

r? @BurntSushi

@rust-highfive rust-highfive assigned BurntSushi and unassigned brson Jul 3, 2017
@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@BurntSushi
Copy link
Member

Seems reasonable to me! Nice find! @bors r+

@bors
Copy link
Contributor

bors commented Jul 3, 2017

📌 Commit fd40567 has been approved by BurntSushi

@Mark-Simulacrum
Copy link
Member

@bors r-

[00:02:54] error[E0308]: mismatched types
[00:02:54]     --> /checkout/src/libstd/f32.rs:1143:17
[00:02:54]      |
[00:02:54] 1143 |             v = NAN;
[00:02:54]      |                 ^^^ expected u32, found f32
[00:02:54]      |
[00:02:54]      = help: here are some functions which might fulfill your needs:
[00:02:54]              - .to_bits()
[00:02:54] 
[00:02:54] error[E0308]: mismatched types
[00:02:54]     --> /checkout/src/libstd/f64.rs:1058:17
[00:02:54]      |
[00:02:54] 1058 |             v = NAN;
[00:02:54]      |                 ^^^ expected u64, found f64
[00:02:54]      |
[00:02:54]      = help: here are some functions which might fulfill your needs:
[00:02:54]              - .to_bits()
[00:02:54] 
[00:02:55] error: aborting due to previous error(s)

@est31 est31 force-pushed the nan_cross_platform branch 2 times, most recently from fbbaa71 to a1a3da5 Compare July 3, 2017 15:11
IEEE 754-1985 specifies the encoding of NaN floating point numbers,
but while it mentions that NaNs can be subdivided into signaling
and quiet ones, it doesn't fix the encoding of signaling NaNs in binary
formats. This led to different implementations (CPUs) having different
encodings. IEEE 754-2008 finally specified the encoding of signaling NaNs
but some architectures are compatible with it, while others aren't.
Certain MIPS and PA-RISC CPUs have different encodings for signaling
NaNs.

In order to have the float <-> binary cast feature of the std library be
portable to them, we don't mask any quiet NaNs like we did before (only
being compliant to IEEE 754-2008 and nothing else), but instead we
simply pass a known good NaN instead.

Note that in the code removed there was a bug; the 64 bit mask for quiet
NaNs should have been `0x0008000000000000` instead of the specified
`0x0001000000000000`.
@Mark-Simulacrum
Copy link
Member

@bors r=BurntSushi

@bors
Copy link
Contributor

bors commented Jul 3, 2017

📌 Commit 3ba0f07 has been approved by BurntSushi

@bors
Copy link
Contributor

bors commented Jul 4, 2017

⌛ Testing commit 3ba0f07 with merge de7f061...

bors added a commit that referenced this pull request Jul 4, 2017
Make sNaN removal code tolerate different sNaN encodings

IEEE 754-1985 specifies the encoding of NaN floating point numbers,
but while it mentions that NaNs can be subdivided into signaling
and quiet ones, it doesn't fix the encoding of signaling NaNs in binary
formats. This led to different implementations (CPUs) having different
encodings. IEEE 754-2008 finally specified the encoding of signaling NaNs
but some architectures are compatible with it, while others aren't.
Certain MIPS and PA-RISC CPUs have different encodings for signaling
NaNs.

In order to have the float <-> binary cast feature of the std library be
portable to them, we don't mask any quiet NaNs like we did before (only
being compliant to IEEE 754-2008 and nothing else), but instead we
simply pass a known good NaN instead.

Note that in the code removed there was a bug; the 64 bit mask for quiet
NaNs should have been `0x0008000000000000` instead of the specified
`0x0001000000000000`.
@bors
Copy link
Contributor

bors commented Jul 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing de7f061 to master...

@bors bors merged commit 3ba0f07 into rust-lang:master Jul 4, 2017
bors added a commit that referenced this pull request Jul 17, 2017
Stabilize float_bits_conv for Rust 1.21

Stabilizes the `float_bits_conv` lib feature for the 1.20 release of Rust. I've initially implemented the feature in #39271 and later made PR #43025 to output quiet NaNs even on platforms with different encodings, which seems to have been the only unresolved issue of the API.

Due to PR #43025 being only applied to master this stabilisation can't happen for Rust 1.19 through the usual "stabilisation on beta" system that is being done for library APIs.

r? @BurntSushi

closes #40470.
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.

6 participants