core: Implement feature float_exact_integer_constants

[okaneco]

Accepted ACP - rust-lang/libs-team#713 (comment)
Tracking issue - #152466

Implement accepted ACP for MAX_EXACT_INTEGER and MIN_EXACT_INTEGER on f16, f32, f64, and f128
Add tests to coretests/tests/floats/mod.rs

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, scottmcm

[tgross35]

You can put this in library/coretests/tests/floats/mod.rs instead, which has macros for handling all the float types. Note that you will need the:

    attrs: {
        f16: #[cfg(any(miri, target_has_reliable_f16))],
        f128: #[cfg(any(miri, target_has_reliable_f128))],
    },

[okaneco]

Thanks, I completely missed that module. I was going to rewrite to take an optional meta attribute for the original macro I wrote. There were a few more roadbumps along the way, had to also add the reliable f16/f128 in doc tests.

Moved accordingly.

[tgross35]

It would be good to strengthen that the property is 1:1 rather than being the max roundtripping integer, since based on discussion this is definitely a tripping point. Perhaps something like the following:

Additionally, if a given x = unknown_int as f128 as i128 satisfies the property MIN_EXACT_INTEGER <= x <= MAX_EXACT_INTEGER, then x is guaranteed to have the same value as unknown_int; that is, an integer within this range has a "one to one" mapping with f128. f128::MAX_EXACT_INTEGER + 1 technically round trips losslessly with f128 but there are multiple values of f128 that may map to f128::MAX_EXACT_INTEGER + 1, so it does not satisfy this "one to one" property.

[okaneco]

Sure, I incorporated what you wrote and tried not to over-complicate the explanation. I was hoping the line above it could suffice but expected some deliberation on the exact wording.

Maximum integer that can be represented exactly in an f32 value, with no other integer converting to the same floating point value.

[tgross35]

Could you add examples to both constants? It would be good to demo the MAX_EXACT_INTEGER, MAX_EXACT_INTEGER + 1, MAX_EXACT_INTEGER + 2 property.

[okaneco]

I was uncertain about adding examples because every other float const seems to lack examples, but they'd come in handy here.

Added examples.

[scottmcm]

I think examples are particularly important here because I would have expected this to be the value without the - 1, so the more opportunities to emphasize its definition the better.

(Things like f32::INFINITY don't have that issue.)

[tgross35]

It's worth mentioning that this is just -MAX_EXACT_INTEGER

[okaneco]

Added.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[tgross35]

r? tgross35

[tgross35]

For the f128 issue, could you try adding a commit that changes the two "un" to "uns" here

rust/compiler/rustc_codegen_cranelift/src/codegen_f16_f128.rs

Lines 191 to 218 in 47611e1

	let name = format!(
	"__float{sign}{size}itf",
	sign = if from_signed { "" } else { "un" },
	size = match from_ty {
	types::I32 => 's',
	types::I64 => 'd',
	types::I128 => 't',
	_ => unreachable!("{from_ty:?}"),
	},
	);
	fx.lib_call(
	&name,
	vec![lib_call_arg_param(fx.tcx, from_ty, from_signed)],
	vec![AbiParam::new(to_ty)],
	&[from],
	)[0]
	} else if from_ty == types::F128 && to_ty.is_int() {
	let ret_ty = if to_ty.bits() < 32 { types::I32 } else { to_ty };
	let name = format!(
	"__fix{sign}tf{size}i",
	sign = if from_signed { "" } else { "un" },
	size = match ret_ty {
	types::I32 => 's',
	types::I64 => 'd',
	types::I128 => 't',
	_ => unreachable!("{from_ty:?}"),
	},
	);

? It looks like cranelift is looking for __fixuntfti but the intrinsic is __fixunstfti.

[tgross35]

Sorry, only the second "un" in that block that goes with __fix. The __float is correct.

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[tgross35]

You can use assert_biteq in the tests which should print the values, and is a bit better for this sort of thing anyway.

For the failing test I suspect we’re hitting the LLVM20 issue on aarch64 with neon. Why it’s showing up I have no idea, we should be using the version with the patch, but maybe the system libs are built with an older version?

After doing a run with assert_biteq to see the values, you could try setting reliable_f128 to false for all LLVM < 21 in llvm_util.rs, rather than the current config (think it’s > 20.1 + neon) and see if that fixes things.

Or if you don’t feel like doing too much digging, you can just mark the f128 tests cfg(false) with a fixme like you originally had, and I’ll chase them down later. Sorry about this mess, I wasn’t aware this would be the first tested exercise of these kind of f128 ops :)

[okaneco]

No worries and thanks for all the guidance.

Adding the assert_biteq tests now. Since they didn't handle the integer types, I resorted to regular asserts and forgot to add anything that checked float equality.

After that, I'll try setting the reliable_f128 as suggested.

[okaneco]

After doing a run with assert_biteq to see the values, you could try setting reliable_f128 to false for all LLVM < 21 in llvm_util.rs, rather than the current config (think it’s > 20.1 + neon) and see if that fixes things.

Actually, that's only set for fp16. For fp128, there's no match arm.

rust/compiler/rustc_codegen_llvm/src/llvm_util.rs

Lines 374 to 399 in 47611e1

	cfg.has_reliable_f16 = match (target_arch, target_os) {
	// LLVM crash without neon <https://github.com/llvm/llvm-project/issues/129394> (fixed in LLVM 20.1.1)
	(Arch::AArch64, _)
	if !cfg.target_features.iter().any(|f| f.as_str() == "neon")
	&& version < (20, 1, 1) =>
	{
	false
	}
	// Unsupported <https://github.com/llvm/llvm-project/issues/94434> (fixed in llvm22)
	(Arch::Arm64EC, _) if major < 22 => false,
	// Selection failure <https://github.com/llvm/llvm-project/issues/50374> (fixed in llvm21)
	(Arch::S390x, _) if major < 21 => false,
	// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
	(Arch::X86_64, Os::Windows) if *target_env == Env::Gnu && *target_abi != Abi::Llvm => false,
	// Infinite recursion <https://github.com/llvm/llvm-project/issues/97981>
	(Arch::CSky, _) if major < 22 => false, // (fixed in llvm22)
	(Arch::Hexagon, _) if major < 21 => false, // (fixed in llvm21)
	(Arch::LoongArch32 | Arch::LoongArch64, _) if major < 21 => false, // (fixed in llvm21)
	(Arch::PowerPC | Arch::PowerPC64, _) if major < 22 => false, // (fixed in llvm22)
	(Arch::Sparc | Arch::Sparc64, _) if major < 22 => false, // (fixed in llvm22)
	(Arch::Wasm32 | Arch::Wasm64, _) if major < 22 => false, // (fixed in llvm22)
	// `f16` support only requires that symbols converting to and from `f32` are available. We
	// provide these in `compiler-builtins`, so `f16` should be available on all platforms that
	// do not have other ABI issues or LLVM crashes.
	_ => true,
	};

rust/compiler/rustc_codegen_llvm/src/llvm_util.rs

Lines 401 to 425 in 47611e1

	cfg.has_reliable_f128 = match (target_arch, target_os) {
	// Unsupported https://github.com/llvm/llvm-project/issues/121122
	(Arch::AmdGpu, _) => false,
	// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
	(Arch::Arm64EC, _) => false,
	// Selection bug <https://github.com/llvm/llvm-project/issues/96432> (fixed in LLVM 20.1.0)
	(Arch::Mips64 | Arch::Mips64r6, _) if version < (20, 1, 0) => false,
	// Selection bug <https://github.com/llvm/llvm-project/issues/95471>. This issue is closed
	// but basic math still does not work.
	(Arch::Nvptx64, _) => false,
	// ABI bugs <https://github.com/rust-lang/rust/issues/125109> et al. (full
	// list at <https://github.com/rust-lang/rust/issues/116909>)
	(Arch::PowerPC | Arch::PowerPC64, _) => false,
	// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
	(Arch::Sparc, _) => false,
	// Stack alignment bug <https://github.com/llvm/llvm-project/issues/77401>. NB: tests may
	// not fail if our compiler-builtins is linked. (fixed in llvm21)
	(Arch::X86, _) if major < 21 => false,
	// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
	(Arch::X86_64, Os::Windows) if *target_env == Env::Gnu && *target_abi != Abi::Llvm => false,
	// There are no known problems on other platforms, so the only requirement is that symbols
	// are available. `compiler-builtins` provides all symbols required for core `f128`
	// support, so this should work for everything else.
	_ => true,
	};

I can try copying that guard, but I don't have a matching issue to leave as a comment...unless we know it was fixed in LLVM 21?

[okaneco]

Only the min test is failing, which deals with negative floats.

It looks like negative floats are saturating to 0u128 instead of being converted back to i128?

        // The minimum integer that converts to a unique floating point
        // value.
        const MIN_EXACT_INTEGER: <Float as TestableFloat>::SInt = Float::MIN_EXACT_INTEGER;

        // Same logic as the `max` test, but we work our way leftward
        // across the number line from (min_exact + 1) to (min_exact - 2).
        let min_plus_one = (MIN_EXACT_INTEGER + 1) as Float as <Float as TestableFloat>::SInt;

        // This does an extra round trip back to float for the second operand in
        // order to print the results if there is a mismatch
        assert_biteq!((MIN_EXACT_INTEGER + 1) as Float, min_plus_one as Float);

---- floats::min_exact_integer_constant::test_f128 stderr ----

thread 'main' (44456) panicked at coretests/tests/floats/mod.rs:1703:9:
l: 0xc06ffffffffffffffffffffffffffffe (0xc06ffffffffffffffffffffffffffffe)
r: 0x00000000000000000000000000000000 (0x00000000000000000000000000000000)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: coretests::floats::min_exact_integer_constant::test_f128
             at ./tests/floats/mod.rs:199:9
---
    floats::min_exact_integer_constant::test_f128

If this last push doesn't work, I think I'll have to ignore the (min) f128 test in the macro.

[okaneco]

Thinking back to the first bug with cranelift, it was having trouble finding the __fixunstfti intrinsic but none of the tests I wrote involved a u128. It should be using __fixtfti to convert to i128.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[okaneco]

@rustbot ready

[tgross35]

@bors r+

[rust-bors]

📌 Commit 6d85426 has been approved by tgross35

It is now in the queue for this repository.

[JonathanBrouwer]

@bors r-
#152616 (comment)

[rust-bors]

Commit 6d85426 has been unapproved.

This PR was contained in a rollup (#152616), which was also unapproved.