Add integer to f16 conversions - #1261
npmccallum wants to merge 3 commits into
Conversation
2cc0b35 to
366b97e
Compare
There was a problem hiding this comment.
Thank you for picking this up! I think the test bits are fine but Juho understood the issue better than me.
Totally fine to supersede my PR but mind picking df3c10a directly? I split off the conflict-y bit.
| // Check rounding against `rustc_apfloat`, a correctly-rounded oracle, | ||
| // by comparing bits directly. The previous heuristic instead cast the | ||
| // float result and its neighbours back to integers and compared error | ||
| // brackets; that saturates `inf` to `iN::MAX` and so misfires for `f16`, | ||
| // where any overflowing conversion correctly yields `inf` (for example | ||
| // `2147483648 as f16`) yet the round-trip flagged it as mis-rounded. | ||
| // | ||
| // Gated on the system routine being available; otherwise `f0` is already | ||
| // the apfloat result and the native comparison below covers it. | ||
| #[cfg($sys_available)] { | ||
| // This makes sure that the conversion produced the best rounding possible, and does | ||
| // this independent of `x as $into` rounding correctly. | ||
| // This assumes that float to integer conversion is correct. | ||
| let y_minus_ulp = <$f_ty>::from_bits(f1.to_bits().wrapping_sub(1)) as $i_ty; | ||
| let y = f1 as $i_ty; | ||
| let y_plus_ulp = <$f_ty>::from_bits(f1.to_bits().wrapping_add(1)) as $i_ty; | ||
| let error_minus = <$i_ty as Int>::abs_diff(y_minus_ulp, x); | ||
| let error = <$i_ty as Int>::abs_diff(y, x); | ||
| let error_plus = <$i_ty as Int>::abs_diff(y_plus_ulp, x); | ||
|
|
||
| // The first two conditions check that none of the two closest float values are | ||
| // strictly closer in representation to `x`. The second makes sure that rounding is | ||
| // towards even significand if two float values are equally close to the integer. | ||
| if error_minus < error | ||
| || error_plus < error | ||
| || ((error_minus == error || error_plus == error) | ||
| && ((f0.to_bits() & 1) != 0)) | ||
| { | ||
| type ApFloat = rustc_apfloat::ieee::$apfloat_ty; | ||
|
|
||
| let expected = if <$i_ty>::SIGNED { | ||
| ApFloat::from_i128(x.try_into().unwrap()).value | ||
| } else { | ||
| ApFloat::from_u128(x.try_into().unwrap()).value | ||
| } | ||
| .to_bits(); | ||
|
|
||
| if u128::from(f1.to_bits()) != expected { | ||
| panic!( | ||
| "incorrect rounding by {}({}): {}, ({}, {}, {}), errors ({}, {}, {})", | ||
| "incorrect conversion by {}({}): apfloat {:#x}, builtins {:#x}", | ||
| stringify!($fn), | ||
| x, | ||
| expected, | ||
| f1.to_bits(), | ||
| y_minus_ulp, | ||
| y, | ||
| y_plus_ulp, | ||
| error_minus, | ||
| error, | ||
| error_plus, | ||
| ); |
366b97e to
b8d7db5
Compare
|
Thanks @tgross35, and thanks for splitting off What I've done instead: kept the equivalent implementation but set you as the author of the conversions commit (I'm listed as co-author), so the primary credit is yours. It's the same six routines; the only deltas from Happy to adjust the attribution however you'd prefer. Thanks again for the original work here. |
| // This makes sure that the conversion produced the best rounding possible, and does | ||
| // this independent of `x as $into` rounding correctly. | ||
| // This assumes that float to integer conversion is correct. | ||
| let y_minus_ulp = <$f_ty>::from_bits(f1.to_bits().wrapping_sub(1)) as $i_ty; | ||
| let y = f1 as $i_ty; | ||
| let y_plus_ulp = <$f_ty>::from_bits(f1.to_bits().wrapping_add(1)) as $i_ty; | ||
| let error_minus = <$i_ty as Int>::abs_diff(y_minus_ulp, x); | ||
| let error = <$i_ty as Int>::abs_diff(y, x); | ||
| let error_plus = <$i_ty as Int>::abs_diff(y_plus_ulp, x); | ||
|
|
||
| // The first two conditions check that none of the two closest float values are | ||
| // strictly closer in representation to `x`. The second makes sure that rounding is | ||
| // towards even significand if two float values are equally close to the integer. | ||
| if error_minus < error | ||
| || error_plus < error | ||
| || ((error_minus == error || error_plus == error) | ||
| && ((f0.to_bits() & 1) != 0)) | ||
| { | ||
| type ApFloat = rustc_apfloat::ieee::$apfloat_ty; | ||
|
|
||
| let expected = if <$i_ty>::SIGNED { | ||
| ApFloat::from_i128(x.try_into().unwrap()).value | ||
| } else { | ||
| ApFloat::from_u128(x.try_into().unwrap()).value | ||
| } | ||
| .to_bits(); |
There was a problem hiding this comment.
So this is replacing the as cast with tests against apfloat?
We should keep testing against the as cast, those come in handy quite a bit for catching ABI or platform bugs that we wouldn't have otherwise. And these functions will get tested against arbitrary-precision math (MPFR) anyway.
The current checks just need to be updated to not produce that error.
There was a problem hiding this comment.
Good call — reverted to the bracket check and kept the native as-cast comparison; agreed those catch ABI/platform issues that an apfloat/MPFR comparison would miss (we just hit exactly that class in the f16→i128 and msvc f128 paths).
The only change now is to skip the bracket when the result is infinite: an integer past the exponent range correctly converts to inf (only reachable for f16 here), and casting inf back to an integer saturates to iN::MAX, which is what produced the spurious failure. The infinite case is still covered by the native comparison below. Both the as-cast path and the apfloat fallback pass across the matrix.
There was a problem hiding this comment.
The current version is still unnecessarily skipping more tests that work currently. Can't the check be fixed rather than skipping things? Probably something like what I mentioned at #729 (comment).
There was a problem hiding this comment.
Thanks—fixed. Finite results keep the existing neighbor check; infinity now uses the two adjacent finite values to verify the overflow threshold and sign. The native as comparison remains. This follows Juho’s explanation in #729. Does this address your concern?
b8d7db5 to
983e12c
Compare
`compat.rs` claims every observable matches nightly, but seventeen cases were
not comparing against nightly at all -- they asserted written-out values,
because nightly cannot currently produce the right answer. Splitting them into
a second file made the substitution visible without making it any less of a
substitution.
Put every case back in `compat.rs` in differential form. A plain `cargo test`
now fails: twelve cases fail to link, four fail at `opt-level = 0`, and one
fails under emulated `avx512fp16`. Those failures are the upstream bugs,
reported where they happen, and they will disappear on their own as the fixes
land.
CI supplies narrow stand-ins so the suite still gates merges. Each is named,
scoped to the steps that run tests, and carries the issue that retires it:
* `--cfg ci_builtin_shims` compiles `tests/shims/int_to_f16.rs`, which defines
`__floattihf` and `__floatuntihf`. No runtime provides them, so `x as f16`
does not link. Implemented from the IEEE 754 rounding rules rather than by
delegating to this crate, so the comparison still contrasts two
implementations; checked against an exhaustive exact oracle over 3020
values. rust-lang/compiler-builtins#1261 removes the need for it.
* `CARGO_PROFILE_TEST_OPT_LEVEL=1`, because nightly's `f16` -> 128-bit
lowering saturates at 64 bits with optimization off.
* `--skip f16_qnan_i16` on the emulated `avx512fp16` leg, where LLVM returns
`i16::MIN` for NaN. Fixed in llvm/llvm-project e1823e8b, which has not
reached nightly's LLVM 23 branch.
None of them changes what a test asserts. The shim is the one that comes
closest, since it stands in for the reference implementation, and it goes away
first.
This comment has been minimized.
This comment has been minimized.
`builtins-test-intrinsics` links every intrinsic LLVM may emit, so a missing symbol surfaces as a link failure rather than silently. It did not cover integer-to-`f16` conversions, which LLVM emits for 128-bit operands: `x as f16` where `x` is a `u128`/`i128` requires `__floatuntihf` / `__floattihf`, and compiler-builtins does not define them. Add all six integer-to-`f16` cases (`i32`/`i64`/`i128` and unsigned). This intentionally fails to link on targets without a fallback provider (e.g. musl, macos) until the following commit adds the routines, matching the failure real user code already hits. On glibc the link only succeeds because libgcc happens to supply the symbols.
LLVM emits `__float{,un}sihf`, `__float{,un}dihf` and `__float{,un}tihf`
when converting 32-, 64- and 128-bit integers to `f16`, but
compiler-builtins never provided them. Code performing `int as f16` fails
to link on any target without another provider of these symbols (for
example musl or macos); glibc targets only link because libgcc supplies
them. These routines are not in LLVM's compiler-rt either.
Implement all six by reusing the existing left-align / round-to-even
machinery. Unlike wider float types, any integer can overflow `f16`'s
exponent range, so each routine clamps to infinity once the rounded
exponent saturates.
Co-authored-by: Nathaniel McCallum <nathaniel.mccallum@amd.com>
Wire the new integer-to-`f16` routines into the `i_to_f` rounding tests and gate them like the other no-sys conversions. The rounding check casts the float result and its neighbours back to integers to bracket the error, which also exercises the `f -> i` cast and catches ABI or platform bugs. For an infinite result, recover the overflow threshold from the two largest finite neighbours instead of casting `inf` and NaN back to integers. This retains the useful `f -> i` coverage while correctly validating the overflow magnitude and sign. `build.rs` sets `no_sys_f16_int_convert` on targets whose system libraries lack these routines (apple, windows, 32-bit x86), matching the existing `no_sys_*` handling; there the apfloat fallback is used, which needs the result narrowed to the float bit width. Co-authored-by: Trevor Gross <tmgross@umich.edu> Suggested-by: Juho Kahala <57393910+quaternic@users.noreply.github.com>
983e12c to
bd542b3
Compare
|
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. |
|
I've been working on getting a second set of eyes other than my own on the test changes, should have another look soon. Could you please clarify what you used AI for? We want to account for what your level of understanding is. The PR description and the commit messages also appear machine-generated which is against our policy https://forge.rust-lang.org/policies/llm-usage.html#-banned, all communication must be handwritten. I am also very much not thrilled that bb42699 appears to have amended a commit authored by me with a machine-generated message that I certainly did not write. Because of the continued confusion here, my plan is to pick the two other commits to #729 rather than the other way around, once approved and messages cleaned up. |
3 participants
Summary
LLVM emits
__float{,un}sihf,__float{,un}dihf, and__float{,un}tihfwhen converting 32-, 64-, and 128-bit integers tof16, but compiler-builtins does not currently provide these symbols.Consequently,
int as f16can fail to link on targets without anotherprovider, including musl and Apple targets. glibc targets generally work
only because libgcc supplies the symbols.
This implements the compiler-builtins side of rust-lang/rust#132614,
which is part of the
f16tracking issue rust-lang/rust#116909.Provenance and credit
The conversion algorithms were originally written by Trevor Gross in
#729.
Trevor's implementation commit,
df3c10a, was based on the November2024 tree and also contained test and ordering changes that conflict with
current
main, so it could not be applied verbatim. This PR preservesthe conversion bodies and ordinary entry points unchanged while adapting
their placement and target configuration to the current tree. The
implementation commit retains Trevor as its primary Git author, with me
listed as co-author.
The test commit incorporates Trevor's original
f16conversion casesand system-library gating work, so he is also listed as co-author there.
The overflow-threshold validation follows Juho Kahala's explanation in
#729, and credits him with a
Suggested-bytrailer.Changes
The PR is divided into three commits:
Exercise the integer-to-
f16link pathsAdd all six signed and unsigned
i32/i64/i128conversions tobuiltins-test-intrinsics.On targets without a fallback provider, this commit reproduces the
missing-symbol failure before the implementation is added. The
intrinsic test crate is excluded from the workspace, so the default
workspace still builds at this commit.
Implement the six conversion routines
Add:
__floatunsihfand__floatsihf__floatundihfand__floatdihf__floatuntihfand__floattihfThe routines reuse the existing left-alignment and round-to-even
machinery. Because the
f16range is narrower than every supportedsource integer type, all six paths handle exponent saturation by
returning the appropriately signed infinity.
Exercise and validate the conversions
Register all six routines with the existing
i_to_ftests and addthe necessary system-library availability configuration.
Rounding validation
The existing
i_to_fcheck casts the result and its neighboring floatvalues back to integers. This independently checks rounding and also
exercises the platform
f -> icasts, which have previously caught ABIand platform-specific problems.
That check cannot directly cast infinity and its NaN neighbor back to an
integer: those casts saturate or produce zero, yielding a meaningless
error bracket.
For an infinite result, the revised test instead:
Equality is accepted as the round-to-even case: the hypothetical
unbounded power of two has an even significand and rounds outward before
the exponent saturates to infinity.
Finite results continue to use the original three-value neighbor
bracket. The native
as-cast comparison is also retained.Platform handling
System-library comparisons are disabled through
no_sys_f16_int_convertwhere the complete routine set is unavailable:On x86_64 UEFI, the
i128/u128entry points use the same split-u64argument ABI now used by the existing
f32andf64conversions.