float to/from bits and classify: update for float semantics RFC #128598

RalfJung · 2024-08-03T09:54:17Z

With rust-lang/rfcs#3514 having been accepted, it is clear that hardware which e.g. flushes subnormal to zero is just non-conformant from a Rust perspective -- this is a hardware bug, or maybe an LLVM backend bug (where LLVM doesn't lower floating-point ops in a way that they have the standardized behavior). So update the comments here to make it clear that we don't have to do any of this, we're just being nice.

Also remove the subnormal/NaN checks from the (unstable) const-version of to/from-bits; they are not needed since we decided with the aforementioned RFC that it is okay to get a different result at const-time and at run-time.

r? @workingjubilee since I think you wrote many of the comments I am editing here.

RalfJung · 2024-08-03T11:03:40Z

@rust-lang/wg-const-eval this also makes it so that to_bits / from_bits in const will expose the bit pattern of NaNs (and subnormals). That matches the accepted float semantics RFC: the exposed bit pattern is guaranteed to be some NaN bit pattern, but which exact bit pattern one gets is not fixed. (These functions are still unstable, though they could be stabilized soon.)

tgross35 · 2024-08-04T00:15:46Z

Would you be able to update f16 and f128 too? If you run into anything too weird there then I can do it separately after this merges.

( 5 files changed, 43 insertions(+), 394 deletions(-) love these kind of diffstats)

RalfJung · 2024-08-04T09:38:45Z

I'll wait for a review before doing that so that I don't have to do the same edits 4 times.

workingjubilee

cool, okay!

sorry for taking so long to review but I had to like... also go over the float rfc again with a fine-tooth comb.

library/core/src/num/f32.rs

workingjubilee · 2024-08-15T02:31:14Z

tests/ui/consts/const-float-bits-conv.rs

+
+    // Check that NaNs roundtrip their bits regardless of signalingness
+    // 0xA is 0b1010; 0x5 is 0b0101 -- so these two together clobbers all the mantissa bits
+    // ...actually, let's just check that these break. :D


This is not new code, I just moved it. ;)

workingjubilee · 2024-08-15T09:51:38Z

library/core/src/num/f64.rs

+        // implementation.
+        //
+        // Unfortunately, there is hardware out there that does not correctly implement the IEEE
+        // float semantics Rust relies on: x87 uses a too large exponent, and some hardware flushes


Suggested change

// float semantics Rust relies on: x87 uses a too large exponent, and some hardware flushes

// float semantics Rust relies on: x87 uses a too-large exponent, and some hardware flushes

RalfJung · 2024-08-15T12:23:18Z

I have done the same edits for f16 and f128, and also replaced the transmutes in classify with to_bits now that that does not lead to recursion any more. The partial_classify functions I found really hard to explain and document separately so I inlined them into their only call site.

RalfJung · 2024-08-15T12:24:25Z

library/core/src/num/f16.rs

+            // we are fine.
+            let b = self.to_bits();
+            match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
+                (0, Self::EXP_MASK) => FpCategory::Infinite,


@tgross35 f16 is somewhat odd here -- it checks for infinity above and then again with the bitwise check here. That's different from f32, which only checks it separately, and f64, which only checks it via the bits. Isn't this arm here dead code because we already checked is_infinite above?

You are right, that is redundant - I don't think that was done with any specific intent.

I will follow what f32 does then since it seems closest to f16.

RalfJung · 2024-08-15T12:26:55Z

( 5 files changed, 43 insertions(+), 394 deletions(-) love these kind of diffstats)

Yeah same. :D
FWIW in principle we could delete a lot more code here and just always do bit-pattern classification. According to our own semantics, that is correct. The LLVM x86-non-SSE codegen path is just buggy (and unlikely to be fixed as nobody who knows how to fix it cares enough bout this target) and that's why we are carrying these work-arounds... and compiling code to instructions that flush subnormals is equally buggy.

workingjubilee

sorry, only noticed this typo on this last scan.

library/core/src/num/f32.rs

library/core/src/num/f64.rs

library/core/src/num/f16.rs

…mant hardware

RalfJung · 2024-08-16T08:11:49Z

Typo fixed :)

workingjubilee · 2024-08-16T13:05:01Z

huzzah!

looks good.
@bors r+ rollup

bors · 2024-08-16T13:05:04Z

📌 Commit 368a4c6 has been approved by workingjubilee

It is now in the queue for this repository.

RalfJung · 2024-08-16T21:12:56Z

Ah! Good point.

Okay, this should fix it then -- I just have it skip the problematic comparison on i586 targets.
@bors r=workingjubilee

bors · 2024-08-16T21:12:58Z

📌 Commit 852b9dd has been approved by workingjubilee

It is now in the queue for this repository.

RalfJung · 2024-08-16T21:13:47Z

@bors r=workingjubilee

bors · 2024-08-16T21:13:50Z

📌 Commit ec1cb80 has been approved by workingjubilee

It is now in the queue for this repository.

beetrees · 2024-08-16T21:18:02Z

tests/ui/consts/const-float-bits-conv.rs

+    const_assert!(f64::from_bits(MASKED_NAN1).to_bits(), MASKED_NAN1);
+    if !has_broken_floats() {
+        const_assert!(f64::from_bits(MASKED_NAN2).to_bits(), MASKED_NAN2);
+    }


Suggested change

const_assert!(f64::from_bits(MASKED_NAN1).to_bits(), MASKED_NAN1);

if !has_broken_floats() {

const_assert!(f64::from_bits(MASKED_NAN2).to_bits(), MASKED_NAN2);

}

if !has_broken_floats() {

const_assert!(f64::from_bits(MASKED_NAN1).to_bits(), MASKED_NAN1);

}

const_assert!(f64::from_bits(MASKED_NAN2).to_bits(), MASKED_NAN2);

As f64 has an even number of explicit significand bits, MASKED_NAN1 will be the signalling NaN.

let's rename these to SIGNALING_NAN and QUIET_NAN shall we?

I am aware of the mips differentiation, you can comment on "according to IEEE754-2008" if you like.

The float semantics RFC states "Rust assumes that the quiet/signalling bit being set to 1 indicates a quiet NaN (QNaN), and a value of 0 indicates a signalling NaN (SNaN)." so it could also be "according to Rust's float semantics".

(As a side note, I don't think f{16,32,64,128}::NAN are explicitly documented as being quiet NaNs anywhere, although this has always been the case and is probably relied upon in practice. Now that the RFC has formally defined what Rust thinks a quiet NaN is, the documentation can probably be updated to guarantee that f{16,32,64,128}::NAN are quiet NaNs, although that's probably a task for a separate PR as it would technically make new stable guarantees.)

tgross35 · 2024-08-16T21:27:24Z

@bors r- per the above

workingjubilee · 2024-08-16T21:50:37Z

thank you ecstatic-morse for explicitly documenting this test is about preserving bits for simple casts even in the signaling/quieted cases, you just saved me from making a very bad suggestion.

workingjubilee

Like this, or so

tests/ui/consts/const-float-bits-conv.rs

also fix typo in const-float-bits-conv

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

RalfJung · 2024-08-17T08:27:31Z

@bors r=workingjubilee

bors · 2024-08-17T08:27:34Z

📌 Commit 5f33085 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-08-17T09:13:16Z

⌛ Testing commit 5f33085 with merge 426a60a...

bors · 2024-08-17T11:37:59Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 426a60a to master...

rust-timer · 2024-08-17T12:53:40Z

Finished benchmarking commit (426a60a): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[5.0%, 5.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.7%, secondary -2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.7%	[-2.4%, -1.3%]	3
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-0.7%	[-2.4%, 2.3%]	4

Cycles

Results (secondary 2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.1%, 2.7%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.1%, 0.0%]	4

Bootstrap: 749.331s -> 750.684s (0.18%)
Artifact size: 339.18 MiB -> 339.13 MiB (-0.01%)

rustbot assigned workingjubilee Aug 3, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 3, 2024

This comment has been minimized.

Sign in to view

RalfJung force-pushed the float-comments branch from e79d748 to 55543a5 Compare August 3, 2024 11:02

RalfJung changed the title ~~float to/from bits and classify: update comments regarding non-conformant hardware~~ float to/from bits and classify: update for float semantics RFC Aug 3, 2024

tgross35 added the A-floating-point Area: Floating point numbers and arithmetic label Aug 7, 2024

workingjubilee approved these changes Aug 15, 2024

View reviewed changes

RalfJung force-pushed the float-comments branch 2 times, most recently from 1aed419 to 29232ca Compare August 15, 2024 12:22

RalfJung commented Aug 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the float-comments branch from 29232ca to b2664cf Compare August 15, 2024 12:28

This comment has been minimized.

Sign in to view

RalfJung force-pushed the float-comments branch from b2664cf to 6df63ee Compare August 15, 2024 12:54

This comment has been minimized.

Sign in to view

RalfJung force-pushed the float-comments branch 2 times, most recently from 9339bdb to eb5f421 Compare August 15, 2024 19:17

workingjubilee reviewed Aug 16, 2024

View reviewed changes

library/core/src/num/f32.rs Outdated Show resolved Hide resolved

library/core/src/num/f64.rs Outdated Show resolved Hide resolved

library/core/src/num/f16.rs Outdated Show resolved Hide resolved

float to/from bits and classify: update comments regarding non-confor…

368a4c6

…mant hardware

RalfJung force-pushed the float-comments branch from eb5f421 to 368a4c6 Compare August 16, 2024 08:11

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2024

bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024

RalfJung force-pushed the float-comments branch from 852b9dd to ec1cb80 Compare August 16, 2024 21:13

beetrees suggested changes Aug 16, 2024

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 16, 2024

workingjubilee reviewed Aug 16, 2024

View reviewed changes

RalfJung and others added 2 commits August 17, 2024 10:26

disable problematic float-conv tests in i586 targets

53e1a2e

also fix typo in const-float-bits-conv

more clear NAN names and fix broken_floats logic

5f33085

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

RalfJung force-pushed the float-comments branch from 5811ab9 to 5f33085 Compare August 17, 2024 08:27

bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 17, 2024

tgross35 mentioned this pull request Aug 17, 2024

Add f16 and f128 to tests/ui/consts/const-float-bits-conv.rs #129190

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 17, 2024

bors merged commit 426a60a into rust-lang:master Aug 17, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 17, 2024

RalfJung deleted the float-comments branch August 17, 2024 12:02

rezwanahmedsami added a commit to rezwanahmedsami/rust that referenced this pull request Aug 17, 2024

rust-lang#128598 rebase done

0aa6075

rezwanahmedsami added a commit to rezwanahmedsami/rust that referenced this pull request Aug 17, 2024

rust-lang#128598 rebase done

e245ed0

	// float semantics Rust relies on: x87 uses a too large exponent, and some hardware flushes
	// float semantics Rust relies on: x87 uses a too-large exponent, and some hardware flushes

float to/from bits and classify: update for float semantics RFC #128598

float to/from bits and classify: update for float semantics RFC #128598

Conversation

RalfJung commented Aug 3, 2024

This comment has been minimized.

RalfJung commented Aug 3, 2024

tgross35 commented Aug 4, 2024 • edited Loading

RalfJung commented Aug 4, 2024

workingjubilee left a 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

RalfJung commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 15, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 16, 2024

workingjubilee commented Aug 16, 2024

bors commented Aug 16, 2024

RalfJung commented Aug 16, 2024

bors commented Aug 16, 2024

RalfJung commented Aug 16, 2024

bors commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beetrees Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

tgross35 commented Aug 16, 2024

workingjubilee commented Aug 16, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 17, 2024

bors commented Aug 17, 2024

rust-timer commented Aug 17, 2024

Overall result: ❌ regressions - no action needed

tgross35 commented Aug 4, 2024 •

edited

Loading

RalfJung commented Aug 15, 2024 •

edited

Loading

beetrees Aug 16, 2024 •

edited

Loading