Rust: Tracking issue for float_bits_conv feature

[est31]

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?

[est31]

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.com-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]

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

[hanna-kruppe]

@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.

[est31]

cc #43025 --- with this merged, could we get this feature stabilized in Rust 1.20?

[BurntSushi]

43055 has been submitted to stabilize this.

@rfcbot fcp merge

[rfcbot]

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

• [x] @BurntSushi
• [x] @Kimundi
• [x] @alexcrichton
• [x] @aturon
• [x] @brson
• [x] @dtolnay
• [x] @sfackler

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.

[alexcrichton]

@rfcbot reviewed

[rfcbot]

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

[rfcbot]

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

[rfcbot]

The final comment period is now complete.

[rfcbot]

The final comment period is now complete.

[Gankra]

Really late to this party. As I note here, the NaN masking is unnecessary from LLVM's perspective.

I'm not totally clear on the platform-specific-NaN-reprs issue, but at least on x86/x64, we can just make from_bits into transmute safely. (probably also aarch64? At least its SIMD units are IEEE 754-2008 compliant, which specs NaN repr). The documentation has been written sufficiently vaguely to allow us to change the implementation to a transmute.

If we wish to do so, this just leaves the following question: do we ignore the existence of non-2008-compliant hardware (transmute on all platforms) or do we allow behaviour to be platform-specific (transmute only on definitely-2008-compliant hardware)?

[sfackler]

I'm certainly on board with just transmuting on architectures we know are okay.

[Amanieu]

Transmuting should be safe on ARM & AArch64.

The only issue is whether a sNaN can cause a trap when it is used. By default floating-point exception are masked on ARM/AArch64, but can be enabled by setting some bits in the fpscr/fpcr register.

These bits also allow enabling exceptions for situations like inexact floating-point rounding, float division by zero, etc. These operations are all supported in safe Rust, so I suggest that the official Rust policy be something along the lines: "Rust assumes that all floating-point operations do not trap."

[sfackler]

I don't think we really care if the operation traps, right? It's not unsafe to do so.

[Amanieu]

We do care if the trap is considered an "observable side-effect" of the operation, since this affects optimization. This would impact dead code elimination for example: you would still have to trigger the trap even if the result of the operation is not used.

[sfackler]

Ah sure that makes sense.

[est31]

@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]

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]

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]

FWIW, that change originally came from here: https://github.com/llvm-mirror/llvm/commit/50b2ca4c73c614574a847c43ad9b87301367c59b

[sunfishcode]

@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.

[est31]

cc #46246