Skip to content

Tracking issue for float_bits_conv feature #40470

@est31

Description

@est31
Member

Tracking issue for the conversion float_bits_conv feature, added by PR #39271.

The main topic during the PR's thread has been about how to handle signaling NaNs, and the final decision was to silently mask them, especially as LLVM behaviour regarding them is not well specified.

Another issue raised by @dwrensha was that sNaNs are not encoded consistently. IEEE 754-2008 (section 8.2.1 in the latest draft) specifies quiet NaNs to have their first bit of the trailing significand field set to 1, for signaling NaNs it should be set to 0. Some MIPS based platforms have it reversed. Should they be cfg'd out?

Activity

added
B-unstableBlocker: Implemented in the nightly compiler and unstable.
T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.
on Mar 13, 2017
est31

est31 commented on Apr 19, 2017

@est31
MemberAuthor

Hmm, seems this feature has broken the ieee754 crate:

error: use of unstable library feature 'float_bits_conv': recently added (see issue #40470)
cargo/registry/src/github.tiyicn.workers.dev-1ecc6299db9ec823/ieee754-0.2.1/src/lib.rs:458:17
    |
458 |                 Self::from_bits(
    |                 ^^^^^^^^^^^^^^^
...
610 | mk_impl!(f64, u64, i16, u16, u64, 11, 52);
    | ------------------------------------------ in this macro invocation

Apparently you can't shadow functions with a trait: https://is.gd/oKiVZH (the first assert works, but the second fails)

est31

est31 commented on Apr 19, 2017

@est31
MemberAuthor

Is the issue above something we have to fix, or rather something the ieee754 crate has to?

hanna-kruppe

hanna-kruppe commented on Apr 30, 2017

@hanna-kruppe
Contributor

@est31 RFC 1105 laid out that adding inherent methods is a minor change so it's up to the team(s) to decide whether we're okay with the practical impact of the change.

25 remaining items

sfackler

sfackler commented on Nov 13, 2017

@sfackler
Member

Ah sure that makes sense.

est31

est31 commented on Nov 13, 2017

@est31
MemberAuthor

@gankro

As I note here, the NaN masking is unnecessary from LLVM's perspective.

very interesting! We need more people like you :).

I am personally okay with anything (the documentation was made vague purposefully!), but before we commit to anything that we can't reverse (e.g. making docs super clear that it is only a transmute), I'd love to get @gankro 's LLVM patch merged upstream so that it has been reviewed by LLVM reviewers and deemed okay.

sunfishcode

sunfishcode commented on Nov 13, 2017

@sunfishcode
Member

Unfortunately, it is still necessary from LLVM's perspective; see https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/InstructionSimplify.cpp#L4142 for example. I support changing LLVM's behavior here, but this isn't just a case of fixing mistaken documentation; it's changing the policy, so we need to be careful.

Gankra

Gankra commented on Nov 13, 2017

@Gankra
Contributor

Interesting. That transformation is justified by sNaN being bad, but in fact doesn't look for sNaN. Rather it looks for undef, and up to this point we've fairly conservatively considered doing ~anything with undef to be UB for Rust (because undef is poorly defined and we don't want to couple with LLVM semantics anyway).

As such, the existence of that transformation shouldn't actually be a barrier to us making this change? (we should still fix llvm as well)

jrmuizel

jrmuizel commented on Nov 13, 2017

@jrmuizel
Contributor

FWIW, that change originally came from here: llvm-mirror/llvm@50b2ca4

sunfishcode

sunfishcode commented on Nov 15, 2017

@sunfishcode
Member

@gankro That's a good point. Allowing Rust to produce sNaNs in more places doesn't break those kinds of optimizations. And I'm not aware of any optimizations that explicitly check for sNaNs, and I've checked all the usual suspects. So this change seems ok.

added 2 commits that reference this issue on Nov 24, 2017
est31

est31 commented on Nov 24, 2017

@est31
MemberAuthor
added a commit that references this issue on Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    B-unstableBlocker: Implemented in the nightly compiler and unstable.T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.final-comment-periodIn the final comment period and will be merged soon unless new substantive objections are raised.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @alexcrichton@Amanieu@jrmuizel@BurntSushi@Gankra

      Issue actions

        Tracking issue for float_bits_conv feature · Issue #40470 · rust-lang/rust