Int <-> float conversion broken by #192

[est31]

#192 seems to have broken conversion between 128 bit ints and floats (rust-lang/rust#44785):

[01:01:43] ------------------------------------------
[01:01:43] stderr:
[01:01:43] ------------------------------------------
[01:01:43] thread 'main' panicked at 'assertion failed: `(left == right)`
[01:01:43]   left: `0`,
[01:01:43]  right: `547625059298595101446575784722432`', /checkout/src/test/run-pass/u128.rs:56:4
[01:01:43] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:01:43] 
[01:01:43] ------------------------------------------

cc @alexcrichton

[est31]

I'm working on the issue

[est31]

Hmmm, my attempt to isolate this failed:

#![feature(compiler_builtins_lib)]
#![feature(i128_type, test)]

extern crate test;
use test::black_box as b;

extern "C" {
    pub fn __floatuntisf(i: u128) -> f32;
    pub fn __floatuntidf(i: u128) -> f64;
    pub fn __fixunssfti(f: f32) -> u128;
    pub fn __fixunsdfti(f: f64) -> u128;
}

#[test]
fn float_int_conv() {
    unsafe {
        let l :u128 = b(432 << 100);
        assert_eq!(__fixunssfti(__floatuntisf(l)), l);
        assert_eq!(__fixunsdfti(__floatuntidf(l)), l);
    }
}

... works fine.

[est31]

So I could reproduce the issue by doing optimize-tests = false in config.toml and ./x.py test src/test/run-pass --test-args 128 --stage 1. If you swap the --stage 1 by --stage 0, its not reproducible. Very weird.

[aidanhs]

Unsure if related, but I'm attempting to build the latest commit of rust and am getting

warning: this expression will panic at run-time
  --> src/rustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/conv.rs:56:21
   |
56 |             if (a & (1 << mant_dig)) != 0 {
   |                     ^^^^^^^^^^^^^^^ attempt to shift left with overflow
...
80 |         int_to_float!(i, i32, f64)
   |         -------------------------- in this macro invocation

warning: this expression will panic at run-time
   --> src/rustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/conv.rs:56:21
    |
56  |             if (a & (1 << mant_dig)) != 0 {
    |                     ^^^^^^^^^^^^^^^ attempt to shift left with overflow
...
112 |         int_to_float!(i, u32, f64)
    |         -------------------------- in this macro invocation

[est31]

@aidanhs with the revert (#198), my changes shouldn't have touched that file at all. git diff master..c781759498617ed060dcd6856390d0bd59bc5226 src/float/conv.rs confirms that.

[aidanhs]

Oh indeed, it was more that master of rust is complaining about something to do with type conversions. I didn't read this issue carefully enough to realise it's exclusively about 128bit conversions.

[AaronKutch]

Does the problem still exist? I just added a bunch of integer to float tests that indicate all of them should work.

[est31]

@AaronKutch does it exist if you re-apply the changed by #192 (or just revert #198)? As it's years ago I have forgotten what the refactor was about, but it should not have broken anything. Maybe after the fix to rust-lang/rust#10184 this is fixed as well, but one can't know without running the tests with the refactor applied.

[AaronKutch]

I wish I had seen this refactor earlier, I will adapt it to the modern compiler-builtins.

[AaronKutch]

I figured out what the problem probably was with your refactor. You see, sometimes the integer being casted to is larger than the integer representing the float, and sometimes it is the other way around. Because of this, one or the other case would lose bits unless you accounted for both:

    // The larger integer has to be casted into, or else the shift overflows
    let r: I = if F::Int::BITS < I::BITS {
        let tmp: I = if exp < sig_bits {
            f.imp_frac().cast() >> (sig_bits - exp).cast()
        } else {
            f.imp_frac().cast() << (exp - sig_bits).cast()
        };
        tmp
    } else {
        let tmp: F::Int = if exp < sig_bits {
            f.imp_frac() >> (sig_bits - exp).cast()
        } else {
            f.imp_frac() << (exp - sig_bits).cast()
        };
        tmp.cast()
    };

[AaronKutch]

This has been fixed by #397. There are float conversion problems related to an LLVM bug, but that is tracked in another issues