Make float::from_bits transmute #46012

Gankra · 2017-11-15T20:10:30Z

See commit message for details.

See also this discussion here: #40470 (comment)

(may require libs team discussion before merging)

Gankra · 2017-11-15T20:10:56Z

Gankra · 2017-11-15T20:14:14Z

It's possible this is also valid on powerpc/sparc64/asmjs but I'm keeping this down to the platforms where I know this works and I actually care about them. :)

sfackler · 2017-11-15T20:24:49Z

I'm definitely on board with this, but I think we should get everyone's eyes on it.

@rfcbot fcp merge

rfcbot · 2017-11-15T20:24:50Z

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

This is the mirror commit to rust-lang/rust#46012

sfackler · 2017-11-15T21:03:15Z

src/libstd/f32.rs

-            // The check above only assumes IEEE 754-1985 to be
-            // valid.
-            v = unsafe { ::mem::transmute(NAN) };
+        if cfg!(not(any(


Can you add comments here and below noting why we only do this on some platforms?

The existing comment seems to address it, no?

The existing comment just says why we reset to a "canonical" NaN instead of just masking off the signal bit.

"While IEEE 754-2008 specifies encodings for quiet NaNs and signaling ones, certain MIPS and PA-RISC CPUs treat signaling NaNs differently."

Err wait, I think I don't follow what you're asking/saying?

The documentation to this function states that "due to the implications of Rust's safety promises being uncertain, if the representation of a signaling NaN "sNaN" float is passed to the function, the implementation is allowed to return a quiet NaN instead".

Someone looking at the implementation may wonder why those questions about safety promises are not a problem on these four architectures, but are on others. There should be a comment saying why this adjustment isn't needed.

I would instead suggest changing the documentation to be "due to some platforms not conforming to IEEE 758-2008..."

I don't understand what IEEE 758-2008 compliance has to do with LLVM treating sNaN as UB or not.

Gankra · 2017-11-15T22:39:22Z

I have changed the PR to transmute on all platforms. The platform-specific cfg was based on me getting confused as a result of the implementation, documentation, and example all being inconsistent with eachother. See commit message for details.

This is the mirror commit to rust-lang/rust#46012

hanna-kruppe · 2017-11-15T22:45:47Z

Should the weasel words around sNaN be removed, i.e., should we commit to not masking sNaN?

And if so, should the the docs also include the note about non-IEEE platforms that's in the commit, to warn people against using it for platform-independent de-/serialization?

Gankra · 2017-11-15T22:54:57Z

Note that the platforms in question are IEEE 758-1985 compliant. It's just how to interpret the signaling bit wasn't spec'd until the 2008 version. Upon reflection, this implementation is maximally good at network interop if you're doing NaN-boxing that just slurps up the mantissa bits. It's not as good if you're specifically just caring about whether the NaN signals or not.

The alternative would be to preserve signaling-ness, at the cost of clobbering the signaling bit across platforms.

On balance my submitted implementation seems the most useful, and if the alternative behaviour is desired, then it should be a seperate function (since this behaviour is still desirable).

In practice, I expect no one will notice or care.

sfackler · 2017-11-16T00:01:54Z

I think the guarantees we want to document are that:

Non-NaN values will always round trip exactly between any set of architectures.
NaN values will remain NaN after a round trip between any set of architectures.
All values will always round trip exactly between the same architecture.

Does that seem right?

Gankra · 2017-11-16T00:03:55Z

That is a reasonably minimal specification that I would be happy with, yes. (pushed test changes)

sfackler · 2017-11-16T00:45:57Z

All values will always round trip exactly between the same architecture.

I think we also want to guarantee this, right?

BurntSushi · 2017-11-16T12:51:39Z

@gankro Thanks for all the leg work on this. :-) I admit, I still feel a little squishy over this, but folks with more experience than me on these matters seem quite confident, so I'm content to move forward.

alexcrichton · 2017-11-16T21:17:38Z

I think the docs may want to be updated here as well, but could we perhaps word them in such a way that implies that despite the current behavior we may change things in the future if necessary? (in that do we really trust LLVM to never change in this regard?)

Gankra · 2017-11-20T19:40:07Z

Willing to change docs if desired (though they are still technically correct), but I'd like to get sign off from the rest of the libs team before digging into that. (the impl should be ~independent)

rfcbot · 2017-11-20T21:54:15Z

🔔 This is now entering its final comment period, as per the review above. 🔔

sfackler · 2017-11-23T20:29:34Z

We discussed this at the last libs meeting and decided that the guarantees in #46012 (comment) seem reasonable.

Gankra · 2017-11-23T21:22:16Z

After some discussion on IRC, it seems like it would be better to simply state "it's just transmute", and explain the implications of this. This ends up providing basically the same guarantees as the libs team agreed to (except guaranteeing that cross-platform transfer is in fact bit-exact).

Pushed updated docs.

sfackler · 2017-11-23T21:26:03Z

src/libstd/f32.rs

+    /// to interpret the NaN signaling bit wasn't actually specified. Most platforms
+    /// (notably x86 and ARM) picked the interpretation that was ultimately 
+    /// standardized in 2008, but some didn't (notably MIPS). As a result, all
+    /// signaling NaNs on MIPS are quiet NaNs on x86, and vice-versa.


Is this really the case, or is it that the signalingness is selected by some payload bit in the other encoding?

They literally agree on the bit, but one says 1=signaling, and the other says 0=signaling.

sfackler · 2017-11-23T21:51:06Z

Build's red, but r=me otherwise.

…ect this). The current implementation/documentation was made to avoid sNaN because of potential safety issues implied by old/bad LLVM documentation. These issues aren't real, so we can just make the implementation transmute (as permitted by the existing documentation of this method). Also the documentation didn't actually match the behaviour: it said we may change sNaNs, but in fact we canonicalized *all* NaNs. Also an example in the documentation was wrong: it said we *always* change sNaNs, when the documentation was explicitly written to indicate it was implementation-defined. This makes to_bits and from_bits perfectly roundtrip cross-platform, except for one caveat: although the 2008 edition of IEEE-754 specifies how to interpet the signaling bit, earlier editions didn't. This lead to some platforms picking the opposite interpretation, so all signaling NaNs on x86/ARM are quiet on MIPS, and vice-versa. NaN-boxing is a fairly important optimization, while we don't even guarantee that float operations properly preserve signalingness. As such, this seems like the more natural strategy to take (as opposed to trying to mangle the signaling bit on a per-platform basis). This implementation is also, of course, faster.

Gankra · 2017-11-23T21:56:20Z

Fixed? (tidy whitespace)

sfackler · 2017-11-23T22:57:31Z

@bors r+

bors · 2017-11-23T22:57:32Z

📌 Commit 439576f has been approved by sfackler

This is the mirror commit to rust-lang/rust#46012

bors · 2017-11-24T06:29:34Z

⌛ Testing commit 439576f with merge 078fbe4...

Make float::from_bits transmute See commit message for details. See also this discussion here: #40470 (comment) (may require libs team discussion before merging)

kennytm · 2017-11-24T09:05:05Z

@bors retry

The AppVeyor dist i686-pc-windows-msvc job was "cancelled by user" with this message but the failure is not notified to GitHub:

Timeout performing HVALS account_clouds_0, inst: 1, mgr: PrepareActiveSockets, err: never, queue: 4, qu: 0, qs: 2, qc: 2, wr: 0, wq: 0, in: 0, ar: 0, clientName: AppVeyor-Appveyor.Worker_IN_1, IOCP: (Busy=1,Free=999,Min=200,Max=1000), WORKER: (Busy=24,Free=32743,Min=200,Max=32767), Local-CPU: 4.09% (Please take a look at this article for some common client-side issues that can cause timeouts: https://github.com/StackExchange/StackExchange.Redis/tree/master/Docs/Timeouts.md)

bors · 2017-11-24T10:06:16Z

⌛ Testing commit 439576f with merge 85d50ce...

Make float::from_bits transmute See commit message for details. See also this discussion here: #40470 (comment) (may require libs team discussion before merging)

bors · 2017-11-24T12:38:06Z

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 85d50ce to master...

Gankra · 2017-11-24T19:54:29Z

The Curse Is Lifted

This is the mirror commit to rust-lang/rust#46012

rust-highfive assigned sfackler Nov 15, 2017

sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 15, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 15, 2017

Gankra added a commit to Gankra/byteorder that referenced this pull request Nov 15, 2017

make int->float conversion a transmute on x86/x64/ARM/aarch64

ca591ce

This is the mirror commit to rust-lang/rust#46012

Gankra mentioned this pull request Nov 15, 2017

make int->float conversion a transmute BurntSushi/byteorder#105

Merged

sfackler reviewed Nov 15, 2017

View reviewed changes

kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Nov 15, 2017

Gankra force-pushed the float-conv-transmute branch from 1154b87 to 8d3e910 Compare November 15, 2017 22:37

Gankra changed the title ~~Make float::from_bits transmute on x86/x64/ARM/aarch64~~ Make float::from_bits transmute Nov 15, 2017

Gankra added a commit to Gankra/byteorder that referenced this pull request Nov 15, 2017

make int->float conversion a transmute on x86/x64/ARM/aarch64

4673fc2

This is the mirror commit to rust-lang/rust#46012

Gankra added a commit to Gankra/byteorder that referenced this pull request Nov 15, 2017

make int->float conversion a transmute

f847070

This is the mirror commit to rust-lang/rust#46012

Gankra force-pushed the float-conv-transmute branch from 8d3e910 to 8852757 Compare November 15, 2017 22:49

Gankra force-pushed the float-conv-transmute branch from 8852757 to 6f2cf4e Compare November 16, 2017 00:02

Gankra force-pushed the float-conv-transmute branch from 6f2cf4e to 3bc8a61 Compare November 16, 2017 02:59

Gankra mentioned this pull request Nov 17, 2017

Deserializer masks out signaling nans servo/webrender#2027

Closed

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 20, 2017

aturon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 21, 2017

Gankra force-pushed the float-conv-transmute branch from 3bc8a61 to 5e47285 Compare November 23, 2017 21:20

sfackler reviewed Nov 23, 2017

View reviewed changes

Gankra force-pushed the float-conv-transmute branch from 5e47285 to 439576f Compare November 23, 2017 21:56

sfackler added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 23, 2017

Gankra added a commit to Gankra/byteorder that referenced this pull request Nov 24, 2017

make int->float conversion a transmute

0b1e3ef

This is the mirror commit to rust-lang/rust#46012

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2017

bors merged commit 439576f into rust-lang:master Nov 24, 2017

BurntSushi pushed a commit to BurntSushi/byteorder that referenced this pull request Nov 29, 2017

make int->float conversion a transmute

1b1f206

This is the mirror commit to rust-lang/rust#46012

Amanieu mentioned this pull request Jan 16, 2018

Untagged unions (tracking issue for RFC 1444) #32836

Closed

5 tasks

ecstatic-morse mentioned this pull request Jun 12, 2020

i686 floating point behavior does not agree with unit tests in debug mode #73288

Closed

Make float::from_bits transmute #46012

Make float::from_bits transmute #46012

Conversation

Gankra commented Nov 15, 2017 • edited Loading

Gankra commented Nov 15, 2017

Gankra commented Nov 15, 2017 • edited Loading

sfackler commented Nov 15, 2017

rfcbot commented Nov 15, 2017 • edited by Kimundi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Nov 15, 2017

hanna-kruppe commented Nov 15, 2017

Gankra commented Nov 15, 2017 • edited Loading

sfackler commented Nov 16, 2017 • edited Loading

Gankra commented Nov 16, 2017

sfackler commented Nov 16, 2017

BurntSushi commented Nov 16, 2017

alexcrichton commented Nov 16, 2017

Gankra commented Nov 20, 2017

rfcbot commented Nov 20, 2017

sfackler commented Nov 23, 2017

Gankra commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Nov 23, 2017

Gankra commented Nov 23, 2017

sfackler commented Nov 23, 2017

bors commented Nov 23, 2017

bors commented Nov 24, 2017

kennytm commented Nov 24, 2017

bors commented Nov 24, 2017

bors commented Nov 24, 2017

Gankra commented Nov 24, 2017

Gankra commented Nov 15, 2017 •

edited

Loading

Gankra commented Nov 15, 2017 •

edited

Loading

rfcbot commented Nov 15, 2017 •

edited by Kimundi

Loading

Gankra commented Nov 15, 2017 •

edited

Loading

sfackler commented Nov 16, 2017 •

edited

Loading