Add classify and related methods for f16 and f128 #127020

tgross35 · 2024-06-27T07:24:13Z

Also constify some functions where that was blocked on classify being available.

r? libs

tgross35 · 2024-06-27T09:16:32Z

@rustbot label +F-f16_and_f128

beetrees

Just a couple of nits with the comments to make them more relevant to the specific types.

library/core/src/num/f128.rs

beetrees · 2024-06-27T19:42:37Z

library/core/src/num/f16.rs

+        // On some processors, in some cases, LLVM will "helpfully" lower floating point ops,
+        // in spite of a request for them using standard IEEE types, to things like x87 operations.
+        // These have an f64's mantissa, but can have a larger than normal exponent.
+        // FIXME(jubilee): Using x87 operations is never necessary in order to function
+        // on x86 processors for Rust-to-Rust calls, so this issue should not happen.
+        // Code generation should be adjusted to use non-C calling conventions, avoiding this.


On x86, LLVM appears to correctly round f16s between each operation even when SSE is disabled and x87 operations are used (when SSE is disabled they are passed and returned like u16s), so x87 shouldn't have any issues of this kind with f16. You could change this (and the various other x87 mentions in this file) to instead mention WASM, which has issues with using f32 precision when f16 is used (llvm/llvm-project#96437 and llvm/llvm-project#96438), although since the examples are mainly illustrative, I wouldn't think this is a blocker or anything.

I reworded this as well as the f128 version. If you have specific wording that is more accurate, I can update it again.

I also removed partial_classify from f128 and switched to only use classify_bits, since I don't think there is any case where that can wind up wrong because of extension/truncation, unlike the other types. Let me know if that is inaccurate.

The wording looks good. There's still a paragraph about x87 in f16::from_bits that you can probably just remove (or mention the WASM issues there instead).

You can replace the entire f128::classify() implementation with Self::classify_bits(mem::transmute(self)): as you say, there aren't the same excess precision problems that other floating-point types have, and just classifying the bits will save on calls to compiler builtins.

Even better, I did change to just using classify_bits.

The f16 paragraph should be gone now, just lost that change in my rebase.

library/core/src/num/f128.rs

joboet · 2024-06-28T12:10:24Z

I know too little about the current state of const floating point to confidently review this.
r? @workingjubilee maybe you do?

library/core/src/num/f16.rs

workingjubilee

Hi, can you doublecheck if these doctests are correctly configured-off?

library/core/src/num/f128.rs

library/core/src/num/f16.rs

library/core/src/num/f128.rs

tgross35 · 2024-07-14T22:36:10Z

@workingjubilee they are all block-level #[cfg(...)] { ... } rather than module-level #![cfg(...)] because #![cfg(...)] seems to require a complementary #![cfg(not(...))] fn main() {}, so it just looks cleaner. Also the spacing is consistent with the existing f16/f128 doctest functions (which do I think looks nicer when reading the code because it keeps all the whole-doctest config spaced away from the code, but 🤷 )

workingjubilee · 2024-07-14T22:42:11Z

library/core/src/num/f16.rs

+        // SAFETY: `u16` is a plain old datatype so we can always transmute to it.
+        // ...sorta.
+        //
+        // It turns out that at runtime, it is possible for a floating point number
+        // to be subject to a floating point mode that alters nonzero subnormal numbers
+        // to zero on reads and writes, aka "denormals are zero" and "flush to zero".
+        //
+        // And, of course evaluating to a NaN value is fairly nondeterministic.
+        // More precisely: when NaN should be returned is knowable, but which NaN?
+        // So far that's defined by a combination of LLVM and the CPU, not Rust.
+        // This function, however, allows observing the bitstring of a NaN,
+        // thus introspection on CTFE.
+        //
+        // In order to preserve, at least for the moment, const-to-runtime equivalence,
+        // we reject any of these possible situations from happening.
+        #[rustc_const_unstable(feature = "const_float_bits_conv", issue = "72447")]
+        const fn ct_f16_to_u16(ct: f16) -> u16 {
+            // FIXME(f16_f128): we should use `.classify()` like `f32` and `f64`, but we don't
+            // yet want to rely on that on all platforms (ABI issues). So just classify the
+            // bits instead.
+
+            // SAFETY: this direction is a POD transmutation. We just can't return it unless
+            // it is normal, infinite, or zero.
+            let bits = unsafe { mem::transmute::<f16, u16>(ct) };
+            match f16::classify_bits(bits) {
+                FpCategory::Nan => {
+                    panic!("const-eval error: cannot use f16::to_bits on a NaN")
+                }
+                FpCategory::Subnormal => {
+                    panic!("const-eval error: cannot use f16::to_bits on a subnormal number")
+                }
+                FpCategory::Infinite | FpCategory::Normal | FpCategory::Zero => bits,
+            }
+        }


So the reason that I did it the other way around is that if we transmute a float, or do an operation on it otherwise, LLVM may change its mind about what the bits were, which makes me leery of classifying the bits post-facto like this.

I do think that this would be better, but unfortunately calling .classify() is just nondeterministic on x86 because of ABI bugs. So while this depends on LLVM's consistency, I think it is better than producing a probably wrong answer (and the results are correct at this time on at least x86 and aarch64). At least to get things off the ground.

Anyway that's what the FIXME is supposed to be for, but happy adjust as you prefer.

Right.

Can we be more specific in the comment than "ABI issues", then? Having done a lot of research on them, I feel like they're a giant umbrella of handwavium that people shove stuff under. "classify is nondet on x86, even before we involve the MXCSR" is fine.

Const to_bits isn't strictly needed at this time of course, I just included it because it unblocks a surprising number of tests (especially clippy), either directly or through other const functions that rely on it.

Missed your comment, I updated this to be more exact.

workingjubilee · 2024-07-14T22:48:42Z

library/core/src/num/f128.rs

+            // SAFETY: this direction is a POD transmutation. We just can't return it unless
+            // it is normal, infinite, or zero.


We already stated we don't want to return it and why, so minimize the SAFETY comment, assuming it is correct ( but see my other remark here: https://github.com/rust-lang/rust/pull/127020/files#r1677218387 ).

Suggested change

// SAFETY: this direction is a POD transmutation. We just can't return it unless

// it is normal, infinite, or zero.

// SAFETY: this is a POD transmutation

alternately,

Suggested change

// SAFETY: this direction is a POD transmutation. We just can't return it unless

// it is normal, infinite, or zero.

// SAFETY: all 65536 bitpatterns can go in, all 65536 bitpatterns can go out

You mean all 340282366920938463463374607431768211456 bit patterns in this file :D updated to the simpler one.

oh right :^)

workingjubilee · 2024-07-14T22:50:52Z

...oh, I completely missed the trailing {

workingjubilee · 2024-07-14T22:51:44Z

@tgross35 could we put the opening { on its own line to make it stand out, given that we don't want to indent the entire test function?

workingjubilee · 2024-07-15T07:24:40Z

oh yes, it is not.

bors · 2024-07-15T08:28:49Z

⌛ Testing commit 421ca1a with merge e74b992...

tgross35 · 2024-07-15T08:30:08Z

@bors r- same thing

tgross35 · 2024-07-15T08:31:37Z

@bors cancel

These constifications were blocked on classification functions being added. Now that those methods are available, constify them. This brings things more in line with `f32` and `f64`.

tgross35 · 2024-07-15T08:35:49Z

I got the float->int functions last time but not int->float 🤦. Hope this is about the last thing.

@rustbot ready

workingjubilee · 2024-07-15T09:28:53Z

@bors r+ rollup=iffy

bors · 2024-07-15T09:28:55Z

📌 Commit 2393093 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-07-15T17:20:36Z

⌛ Testing commit 2393093 with merge eb72697...

bors · 2024-07-15T19:43:14Z

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

rust-timer · 2024-07-15T20:59:57Z

Finished benchmarking commit (eb72697): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -3.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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-4.3%, -2.1%]	8
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 700.525s -> 699.024s (-0.21%)
Artifact size: 328.65 MiB -> 328.62 MiB (-0.01%)

rustbot assigned joboet Jun 27, 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 Jun 27, 2024

tgross35 marked this pull request as ready for review June 27, 2024 07:24

tgross35 force-pushed the f16-f128-classify branch 3 times, most recently from d095e37 to 6e8e9f2 Compare June 27, 2024 07:48

rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Jun 27, 2024

beetrees reviewed Jun 27, 2024

View reviewed changes

tgross35 force-pushed the f16-f128-classify branch from 6e8e9f2 to a1939c5 Compare June 27, 2024 21:21

fbstj reviewed Jun 27, 2024

View reviewed changes

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

tgross35 force-pushed the f16-f128-classify branch from a1939c5 to 128b8db Compare June 27, 2024 22:27

rustbot assigned workingjubilee and unassigned joboet Jun 28, 2024

beetrees reviewed Jun 28, 2024

View reviewed changes

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

tgross35 force-pushed the f16-f128-classify branch from 128b8db to d8c58a2 Compare July 1, 2024 22:39

workingjubilee requested changes Jul 14, 2024

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2024

workingjubilee reviewed Jul 14, 2024

View reviewed changes

Add classify and related methods for f16 and f128

3a2c0ae

tgross35 force-pushed the f16-f128-classify branch from d8c58a2 to 7258951 Compare July 14, 2024 22:44

workingjubilee reviewed Jul 14, 2024

View reviewed changes

tgross35 force-pushed the f16-f128-classify branch 2 times, most recently from 69afb49 to 00f3417 Compare July 14, 2024 23:15

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 Jul 15, 2024

Mark some f16 and f128 functions unstably const

2393093

These constifications were blocked on classification functions being added. Now that those methods are available, constify them. This brings things more in line with `f32` and `f64`.

tgross35 force-pushed the f16-f128-classify branch from 421ca1a to 2393093 Compare July 15, 2024 08:34

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2024

bors merged commit eb72697 into rust-lang:master Jul 15, 2024

rustbot added this to the 1.81.0 milestone Jul 15, 2024

tgross35 deleted the f16-f128-classify branch July 15, 2024 20:03

This was referenced Jul 15, 2024

Add f16 formatting and parsing #127013

Merged

Add f16 and f128 math functions #127027

Merged

tgross35 mentioned this pull request Jul 18, 2024

Tracking Issue for f16 and f128 float types #116909

Open

92 tasks

		// SAFETY: this direction is a POD transmutation. We just can't return it unless
		// it is normal, infinite, or zero.

	// SAFETY: this direction is a POD transmutation. We just can't return it unless
	// it is normal, infinite, or zero.
	// SAFETY: this is a POD transmutation

	// SAFETY: this direction is a POD transmutation. We just can't return it unless
	// it is normal, infinite, or zero.
	// SAFETY: all 65536 bitpatterns can go in, all 65536 bitpatterns can go out

Uh oh!

Add classify and related methods for f16 and f128 #127020

Add classify and related methods for f16 and f128 #127020

Uh oh!

Conversation

tgross35 commented Jun 27, 2024

Uh oh!

tgross35 commented Jun 27, 2024

Uh oh!

beetrees left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beetrees Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joboet commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgross35 commented Jul 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Jul 14, 2024

Uh oh!

workingjubilee commented Jul 14, 2024

Uh oh!

workingjubilee commented Jul 15, 2024

Uh oh!

bors commented Jul 15, 2024

Uh oh!

tgross35 commented Jul 15, 2024

Uh oh!

tgross35 commented Jul 15, 2024

Uh oh!

tgross35 commented Jul 15, 2024

Uh oh!

workingjubilee commented Jul 15, 2024

Uh oh!

bors commented Jul 15, 2024

Uh oh!

bors commented Jul 15, 2024

Uh oh!

beetrees Jun 27, 2024 •

edited

Loading

joboet commented Jun 28, 2024 •

edited

Loading

workingjubilee Jul 14, 2024 •

edited

Loading