enable const-float-classify test, and test_next_up/down on 32bit x86

[RalfJung]

The test_next_up/down tests have been disabled on all 32bit x86 targets, which goes too far -- they should definitely work on our (tier 1) i686 target, it is only without SSE that we might run into trouble due to #114479. However, I cannot reproduce that trouble any more -- maybe that got fixed by #123351?

The const-float-classify test relied on const traits "because we can", and got disabled when const traits got removed. That's an unfortunate reduction in test coverage of our float functionality, so let's restore the test in a way that does not rely on const traits.

The const-float tests are actually testing runtime behavior as well, and I don't think that runtime behavior is covered anywhere else. Probably they shouldn't be called "const-float", but we don't have a tests/ui/float folder... should I create one and move them there? Are there any other ui tests that should be moved there?

I also removed some FIXME referring to not use x87 for Rust-to-Rust-calls -- that has happened in #123351 so this got fixed indeed. Does that mean we can simplify all that float code again? I am not sure how to test it. Is running the test suite with an i586 target enough?

Cc @tgross35 @workingjubilee

[rustbot]

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

I tried this diff:

diff --git a/library/core/src/num/f32.rs b/library/core/src/num/f32.rs
index 4c089946033..ca2205d38e1 100644
--- a/library/core/src/num/f32.rs
+++ b/library/core/src/num/f32.rs
@@ -662,13 +662,7 @@ pub const fn classify(self) -> FpCategory {
         // hardware flushes subnormals to zero. These are platforms bugs, and Rust will misbehave on
         // such hardware, but we can at least try to make things seem as sane as possible by being
         // careful here.
-        if self.is_infinite() {
-            // A value may compare unequal to infinity, despite having a "full" exponent mask.
-            FpCategory::Infinite
-        } else if self.is_nan() {
-            // And it may not be NaN, as it can simply be an "overextended" finite value.
-            FpCategory::Nan
-        } else {
+        {
             // However, std can't simply compare to zero to check for zero, either,
             // as correctness requires avoiding equality tests that may be Subnormal == -0.0
             // because it may be wrong under "denormals are zero" and "flush to zero" modes.
@@ -683,6 +677,8 @@ pub const fn classify(self) -> FpCategory {
             // I fear losing mantissa data that would have answered that differently.
             let b = self.to_bits();
             match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
+                (0, Self::EXP_MASK) => FpCategory::Infinite,
+                (_, Self::EXP_MASK) => FpCategory::Nan,
                 (0, 0) => FpCategory::Zero,
                 (_, 0) => FpCategory::Subnormal,
                 _ => FpCategory::Normal,
diff --git a/library/core/src/num/f64.rs b/library/core/src/num/f64.rs
index bcef8fe7724..b5ef9e85aac 100644
--- a/library/core/src/num/f64.rs
+++ b/library/core/src/num/f64.rs
@@ -663,9 +663,7 @@ pub const fn classify(self) -> FpCategory {
         //
         // Thus, a value may compare unequal to infinity, despite having a "full" exponent mask.
         // And it may not be NaN, as it can simply be an "overextended" finite value.
-        if self.is_nan() {
-            FpCategory::Nan
-        } else {
+        {
             // However, std can't simply compare to zero to check for zero, either,
             // as correctness requires avoiding equality tests that may be Subnormal == -0.0
             // because it may be wrong under "denormals are zero" and "flush to zero" modes.
@@ -678,6 +676,7 @@ pub const fn classify(self) -> FpCategory {
             let b = self.to_bits();
             match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
                 (0, Self::EXP_MASK) => FpCategory::Infinite,
+                (_, Self::EXP_MASK) => FpCategory::Nan,
                 (0, 0) => FpCategory::Zero,
                 (_, 0) => FpCategory::Subnormal,
                 _ => FpCategory::Normal,

And the tests seem to still pass. But I am not sure if there are more tests you ran, @workingjubilee ? Is it worth trying the bit-based classify implementation again?

I think I'll rather not touch this code for now.^^

[tgross35]

The test_next_up/down tests have been disabled on all 32bit x86 targets, which goes too far -- they should definitely work on our (tier 1) i686 target, it is only without SSE that we might run into trouble due to #114479. However, I cannot reproduce that trouble any more -- maybe that got fixed by #123351?

Could a similar test be tried making sure the calls are extern "C"? I suspect that #123351 did fix things for the default ABI, but it left C untouched.

[tgross35]

The const-float tests are actually testing runtime behavior as well, and I don't think that runtime behavior is covered anywhere else. Probably they shouldn't be called "const-float", but we don't have a tests/ui/float folder... should I create one and move them there? Are there any other ui tests that should be moved there?

I don't really know what a good solution is here. We have a ton of float tests that are quite similar but slightly different, spread out in a bunch of different locations - std tests, UI tests, miri, compiler_builtins, etc. Even the pattern generation we have at https://github.com/rust-lang/rust/tree/a7399ba69d37b019677a9c47fe89ceb8dd82db2d/src/etc/test-float-parse now would be pretty useful for making sure the edge cases are checked.

I know that is entirely unhelpful, just thinking it would be nice if we had a single suite we could run against both const and runtime somehow.

[RalfJung]

Could a similar test be tried making sure the calls are extern "C"? I suspect that #123351 did fix things for the default ABI, but it left C untouched.

This calls a bunch of Rust functions though so the Rust ABI is what we want. What would be the point of using the C ABI here? We know that one is broken...

[tgross35]

Ah, I thought you were still trying to work around the fix to repro the behavior that led to these notes.

[RalfJung]

I tried adding extern "C" to from_bits, next_up, next_down -- that made no difference.

So yeah not sure how to reproduce the failures. I guess we should do a try-build of the relevant runner. Time to figure out how that works...

[RalfJung]

These tests seem to have been disabled on x86 since they got added: #100578. And that goes back to this CI failure. So we should be seeing it on CI if it's still a problem.

[RalfJung]

But the tests passed on CI. So... maybe something changed in LLVM that makes this less of a problem in practice?

[RalfJung]

The job passed, and then the "dist" part failed of course.

So... something else must have changed to make the text_up/down stuff work on x87 now, and even make d6ea15a work.

@workingjubilee how did you test the classify stuff? Do you have a setup that can reproduce a failure when one does "naive" bit-based classification?

[RalfJung]

Anyway that seems orthogonal to this PR -- test_next_up/down were disabled on x86 because they failed on CI; they no longer fail on CI; that should be sufficient to enable these tests again.

[workingjubilee]

@RalfJung I have rebased your PR, I hope that's okay.

[workingjubilee]

As a minor note, it's very important to make "moving some files around" and "changing those files" as separate commits because "diffing files against the previous state and noticing some are very similar" is exactly how git notices some files moved. That's why in the process of rebasing this PR I introduced a pure "movement only" commit.

[workingjubilee]

@bors r+

[bors]

📌 Commit e556c13 has been approved by workingjubilee

It is now in the queue for this repository.

[RalfJung]

As a minor note, it's very important to make "moving some files around" and "changing those files" as separate commits because "diffing files against the previous state and noticing some are very similar" is exactly how git notices some files moved. That's why in the process of rebasing this PR I introduced a pure "movement only" commit.

Yeah, sorry for that... move + small changes usually works fine but then the changes became bigger and bigger.