Add integer to f16 conversions

[tgross35]

These are not present in LLVM's compiler-rt but LLVM does emit them in some cases 1.

[tgross35]

Includes #730 since PPC crashes without it.

[tgross35]

@quaternic would you mind double checking some of my logic here?

With this current implementation, we are getting errors like the following:

thread 'i_to_f::__floatunsihf' (15281862) panicked at builtins-test/tests/conv.rs:103:5:
incorrect rounding by __floatunsihf(2147483648): inf (0x7c00)
f->i bracket: (65504, 4294967295, 0)
errors: (2147418144, 2147483647, 2147483648)

Which is coming from the bit of code here https://github.com/tgross35/compiler-builtins/blob/c462b7b5aa56a2ff3cdbfaf7c19143bbdee17b00/builtins-test/tests/conv.rs#L44-L79. I think the implementation is correct, especially considering the tests against apfloat seem okay. So I think this is an error in the validation algorithm here, which only shows up for f16 because f16::MAX is way closer to 0 (NAN as i*) than to any iN::MAX for N >= 32 (all that we have algorithms for). And that isn't true for any other float type, even f32::MAX as u128 is > u128::MAX / 2.

As I'm typing it out that sounds pretty reasonable, just need to figure out how to patch this error algorithm.

[quaternic]

I think the validation logic is broken in that the apfloat-results that overflowed to infinity are converted back to an integer, specifically the maximum for the type, which should be irrelevant for the casting from integer to float.

The overflow threshold is specifically when the exponent after rounding exceeds the maximum. But the rounding itself is done as if the exponent was unbounded. So for f16:

[src/main.rs:4:5] f16::MAX.next_down() as f32 = 65472.0
[src/main.rs:5:5] f16::MAX as f32 = 65504.0
[src/main.rs:6:5] 65519 as f16 as f32 = 65504.0
[src/main.rs:7:5] 65520 as f16 as f32 = inf

65520 as f16 overflows because it would round to 65536.0 if the exponent range allowed.

[tgross35]

Isn't that represented correctly? The apfloat bit at https://github.com/tgross35/compiler-builtins/blob/c462b7b5aa56a2ff3cdbfaf7c19143bbdee17b00/builtins-test/tests/conv.rs#L29-L37 should be the same as this:

>> :dep rustc_apfloat
   Compiling rustc_apfloat v0.2.3+llvm-462a31f5a5ab
   Compiling smallvec v1.15.1
   Compiling bitflags v2.10.0
>> use rustc_apfloat::ieee::Half
>> use rustc_apfloat::Float;
>> Half::from_i128(65519)
StatusAnd { status: Status(INEXACT), value: 65504(Normal | +[2047] * 2^15) }
>> Half::from_i128(65519).value.to_bits()
31743
>> Half::from_i128(65520)
StatusAnd { status: Status(OVERFLOW | INEXACT), value: +Inf(Infinity | +[0] * 2^16) }
>> Half::from_i128(65520).value.to_bits()
31744

[quaternic]

Lines 48-50 as-cast the f16 values back to integers, which saturates inf to u32::MAX, and NaN to 0, which is where you get the printed line

f->i bracket: (65504, 4294967295, 0)

[tgross35]

Ah, by

the apfloat-results that overflowed to infinity

I thought you were referring to the test above that uses rustc_apfloat. It's probably fine to skip the error_plus < error part of the check if <$f_ty>::wrapping_add(f1.to_bits().wrapping_sub(1)) is infinite then, right?

[tgross35]

@quaternic forgot to follow up here, do you have an idea what would be better to write for the test assertion?

[npmccallum]

@tgross35 — I've opened #1261, which builds on your implementation here to get integer-to-f16 conversions landed. It carries your conversion commit with you as Co-authored-by:, proposes an answer to the test-oracle question you raised in June (assert against rustc_apfloat directly instead of the f→i bracket, which saturates inf and misfires for f16), and adds a builtins-test-intrinsics link case so the missing symbols surface as a link failure. CI is green across the full matrix.

It's meant to supersede this PR, but I'm equally happy to fold the changes back into your branch instead if you'd rather keep it here — whichever you prefer. Thanks for the original work.

[beetrees]

A couple of questions.

r? me

View changes since this review

[beetrees]

Unless I'm missing something, I'm not sure the "(masked portion)" makes sense here, as nowhere is the lower half of i masked. The lower quarter of i_m is masked in the line below. (The code is correct as f16::EXP_BITS <= 8.)

[beetrees]

Not something that has to be changed in this PR, but is there any reason why some conversion do if i == 0 at the start of the function and some do it when calculating the exponent?

[tgross35]

I'm not sure, looks like that came with 7e5768b. Doing that here rather than the beginning makes it branchless, but I wonder if there's still an advantage to the early return? Would be interesting to benchmark.

[tgross35]

Found https://blog.m-ou.se/floats/#removing-the-last-branch which does specifically mention the branchless bit

[rustbot]

Error: Failed to set assignee to beetrees: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[beetrees]

r? me

[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.

[rustbot]

⚠️ Warning ⚠️

• The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

  • 75b0d62

  You can start a rebase with the following commands:

  $ # rebase
  $ git pull --rebase https://github.com/rust-lang/compiler-builtins.git main
  $ git push --force-with-lease