Add f128 int to float conversions

[tgross35]

No description provided.

[tgross35]

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.

[m-ou-se]

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

[tgross35]

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!

[m-ou-se]

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.

[m-ou-se]

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

[tgross35]

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.

[tgross35]

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?

[m-ou-se]

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.

[m-ou-se]

It keeps the highest bit in tact.

[tgross35]

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.

[tgross35]

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

[m-ou-se]

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

[bjorn3]

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

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

[tgross35]

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.

[tgross35]

Yay! Many thanks for the reviews :)