Add f128 int to float conversions by tgross35 · Pull Request #624 · rust-lang/compiler-builtins (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits4 Checks25 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This should be ready now

Cc @m-ou-se since you wrote the original int to float algorithms. The generic function's monos seem to get reasonably similar codegen, running the benchmarks seems like changes are within the noise margin.

tgross35 marked this pull request as ready for review

May 29, 2024 08:34

Haven't looked at it in detail, but was it necessary to make it generic? For f128, all integers up to 64 bit fit into it losslessly without rounding, so those conversions should be nearly trivial, right?

Anyway, happy to review this in detail next week.

r? m-ou-se

Haven't looked at it in detail, but was it necessary to make it generic?

Not strictly, but nine near-identical functions would be a lot. I think the constants are easier to follow than the magic numbers in any case.

Anyway, happy to review this in detail next week.

Thanks!

Having reviewed in detail now, I'm still not convinced we should make this function generic. It now has quite a few different cases to handle, adding complexity. And now it always starts with an "if 0" check, even though a few functions (e.g. u64_to_f32_bits) were branch free (after codegen) before.

Doing each of them separately makes it possible to spot nice optimizations that only apply to specific cases.

For example, in the u32 to f128 case, the lower 64 bits will always be zero, so all the operations can be done on u64 rather than u128, which results in much shorter and faster assembly:

pub fn u32_to_f128_bits(i: u32) -> u128 { if i == 0 { return 0; } let n = i.leading_zeros(); let m = (i as u64) << (17 + n); // High 64 significant bits, with bit 113 still in tact. let e = 16413 - n as u64; // Exponent plus 16383, minus one. let h = (e << 48) + m; // High 64 bits of f128 representation. (h as u128) << 64 // Low bits are always 0. }

Thanks for taking a look. I think I struck a happier medium - moved the redundant parts to generic functions to make things less magic-number-y, but did not combine the algorithms. Also included your suggested u32 -> f128 conversion.

It looks like the system NaN functions are converting to max values rather than zero for f128. Not sure why this would be but I'll disable them and look into it.

@m-ou-se gentle nudge, could you take another look at this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find it more readable when split up over multiple functions like this. For example, the exp function doesn't actually calculate the exponent; it calculates the exponent minus one, because of the trick with the mantissa containing an extra 1 bit that will unconditinally overflow into the exponent later. Separating tricks over multiple functions just makes it harder to follow and review.

That said, if you truly find this more maintainable, I'll be happy to approve this given that the code is well tested and benchmarked.

Comment on lines 25 to 26

/// Shift the integer into the float's mantissa bits. Keep the lowest exponent bit intact.
fn m_base<I: Int, F: Float<Int: CastFrom<I>>>(i_m: I) -> F::Int {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps the highest bit in tact.

Ah this kept falling to the back of my todo list, but I finally got around to it. I changed it so none of the mantissa shifting is hidden in functions, only the values they shift by, so compared to the original it's just easier to understand where the numbers come from. I left the obvious functions exp and repr, also m_adj because that is a tricky pattern used the same way everywhere.

Take a look when you get the chance @m-ou-se, feel free to push on top of this if you feel it's needed.

@m-ou-se could you take another look at this? Last puzzle piece to making f128 testable on all platforms :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me now. Feel free to merge. :)

// floatsisf
pub fn aeabi_i2f(x: i32) -> f32 {
x as f32
pub fn floatsitf(x: i32) -> f128 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need #[cfg(f128_enable)], right?

Edit: Never mind. Missed that this is an example.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do tests in this repo ever get run with cranelift? This "example" is what tests linking when the current crate is used as compiler_builtins rather than the sysroot.

Yay! Many thanks for the reviews :)

Extract some common routines to separate functions in order to deduplicate code and remove some of the magic.

tgross35 added a commit to tgross35/rust that referenced this pull request

Nov 1, 2024