Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{i686,x86_64}-pc-windows-gnu Appveyor builds failing #72

Open
gnzlbg opened this issue Aug 10, 2018 · 28 comments
Open

{i686,x86_64}-pc-windows-gnu Appveyor builds failing #72

gnzlbg opened this issue Aug 10, 2018 · 28 comments
Labels
Blocked-Rust Issues blocked on rust-lang/rust CI-Appveyor Appveyor CI O-Windows P-Medium Unsound Something breaks Rust safety guarantees

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 10, 2018

It appears that incorrect results are produced on windows when the gnu toolchain is used and NaNs are involved.

failures:
---- v64::f32x2_math_cos::cos stdout ----
thread 'v64::f32x2_math_cos::cos' panicked at 'assertion failed: `(left == right)`
  left: `f32x2(-0.00000004371139, -0.00000004371139)`,
 right: `f32x2(NaN, -0.00000004371139)`', src\v64.rs:43:1
---- v64::f32x2_math_fma::fma stdout ----
thread 'v64::f32x2_math_fma::fma' panicked at 'assertion failed: `(left == right)`
  left: `f32x2(1.0, 1.0)`,
 right: `f32x2(NaN, NaN)`', src\v64.rs:43:1
---- v64::f32x2_math_sin::sin stdout ----
thread 'v64::f32x2_math_sin::sin' panicked at 'assertion failed: `(left == right)`
  left: `f32x2(1.0, 1.0)`,
 right: `f32x2(NaN, 1.0)`', src\v64.rs:43:1
---- v64::f32x2_ops_scalar_arith::ops_scalar_arithmetic stdout ----
thread 'v64::f32x2_ops_scalar_arith::ops_scalar_arithmetic' panicked at 'assertion failed: `(left == right)`
  left: `f32x2(NaN, 0.0)`,
 right: `f32x2(0.0, 0.0)`', src\v64.rs:43:1
---- v64::f32x2_ops_vector_arith::ops_vector_arithmetic stdout ----
thread 'v64::f32x2_ops_vector_arith::ops_vector_arithmetic' panicked at 'assertion failed: `(left == right)`
  left: `f32x2(NaN, 0.0)`,
 right: `f32x2(0.0, 0.0)`', src\v64.rs:43:1
failures:
    v64::f32x2_math_cos::cos
    v64::f32x2_math_fma::fma
    v64::f32x2_math_sin::sin
    v64::f32x2_ops_scalar_arith::ops_scalar_arithmetic
    v64::f32x2_ops_vector_arith::ops_vector_arithmetic
test result: FAILED. 1436 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out

cc @retep007

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 10, 2018

The following operations of the f32x2 vector type produce incorrect results on gnu windows targets ({i686, x86_64}-pc-windows-gnu) in debug builds: sin, cos, fma, scalar (+,-,*,/, unary -).

This playground example might be used to reproduce this though (I can't know since I don't have access to the platform - it would be good to know if that reproduces the issue or not to narrow down the root cause):

#![feature(
    repr_simd,
    simd_ffi,
    link_llvm_intrinsics,
    platform_intrinsics
)]

#[repr(simd)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[allow(non_camel_case_types)]
struct f32x2(f32, f32);

extern "platform-intrinsic" {
    fn simd_fma<T>(a: T, b: T, c: T) -> T;
}

#[allow(improper_ctypes)]
extern "C" {
    #[link_name = "llvm.cos.v2f32"]
    fn cos_v2f32(x: f32x2) -> f32x2;
    #[link_name = "llvm.sin.v2f32"]
    fn sin_v2f32(x: f32x2) -> f32x2;
}

fn main() {
    let a = f32x2(1.0, 1.0);
    let b = f32x2(0.0, 0.0);
    let c = f32x2(2.0, 2.0);

    unsafe {
        assert_eq!(simd_fma(a, c, b), c);
        assert_eq!(sin_v2f32(b), b);
        assert_eq!(cos_v2f32(b), a);
    }
}

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 10, 2018

Filled issue upstream: rust-lang/rust#53254

@gnzlbg gnzlbg added the Blocked-Rust Issues blocked on rust-lang/rust label Aug 10, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 10, 2018

This might be related to: #25 which shows a similar failure on apple darwin.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 24, 2018

Maybe @CAD97 can help set this up?

@CAD97
Copy link

CAD97 commented Sep 24, 2018

Why me? 😛 Is it just because I jumped ship from windows-msvc to -gnu? Sure, I'm a CI guy but you already have that set up!
Happy to help though!

The given minimal case passes on the Windows 10 machine I'm writing this from under windows-gnu.

%projects%\untitled>rustup show

active toolchain
----------------

nightly-x86_64-pc-windows-gnu (directory override for '%projects%\untitled')
rustc 1.30.0-nightly (f49f6e73a 2018-09-23)

%projects%\untitled>type src\main.rs
#![feature(
repr_simd,
simd_ffi,
link_llvm_intrinsics,
platform_intrinsics
)]

#[repr(simd)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[allow(non_camel_case_types)]
struct f32x2(f32, f32);

extern "platform-intrinsic" {
    fn simd_fma<T>(a: T, b: T, c: T) -> T;
}

#[allow(improper_ctypes)]
extern "C" {
    #[link_name = "llvm.cos.v2f32"]
    fn cos_v2f32(x: f32x2) -> f32x2;
    #[link_name = "llvm.sin.v2f32"]
    fn sin_v2f32(x: f32x2) -> f32x2;
}

fn main() {
    let a = f32x2(1.0, 1.0);
    let b = f32x2(0.0, 0.0);
    let c = f32x2(2.0, 2.0);

    unsafe {
        assert_eq!(simd_fma(a, c, b), c);
        assert_eq!(sin_v2f32(b), b);
        assert_eq!(cos_v2f32(b), a);
    }

    println!("ok");
}

%projects%\untitled>cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target\debug\untitled.exe`
ok

Running --tests f32x2 on this repo I get the following failures (and it does indeed pass in --release:

failures:
    v64::f32x2_math_cos::cos
    v64::f32x2_math_exp::exp
    v64::f32x2_math_ln::ln
    v64::f32x2_math_powf::powf
    v64::f32x2_math_sin::sin
    v64::f32x2_ops_scalar_arith::ops_scalar_arithmetic
    v64::f32x2_ops_vector_arith::ops_vector_arithmetic

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 24, 2018

Thank you for confirming that the repro fails!

@CAD97
Copy link

CAD97 commented Sep 25, 2018

I see that the upstream LLVM patch has landed. Is there documentation of how to build a local version of the compiler against a development LLVM so I could potentially test to see if the patch effected this issue?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 25, 2018

@CAD97 see this comment: rust-lang/stdarch#513 (comment)

@mati865
Copy link
Contributor

mati865 commented Oct 24, 2018

Rust's llvm already has this fix. I'll see if I can reproduce it later on today.

@mati865
Copy link
Contributor

mati865 commented Oct 24, 2018

64bit Windows 10, latest gnu nightly and the same result as in #72 (comment)

BTW I had to remove deny from src/lib.rs because of dozens of warnings:

warning: variable does not need to be mutable
   --> src\api\reductions\float_arithmetic.rs:170:33
    |
170 |                               let mut v = v0.replace(i, n0);
    |                                   ----^
    |                                   |
    |                                   help: remove this `mut`
    |
   ::: src\v256.rs:48:1
    |
48  | / impl_f!([f32; 8]: f32x8, m32x8 | f32 | test_v256 | x0, x1, x2, x3, x4, x5, x6, x7 |
49  | |         From: i8x8, u8x8, i16x8, u16x8 |
50  | |         /// A 256-bit vector with 8 `f32` lanes.
51  | | );
    | |__- in this macro invocation

New lint or what?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 25, 2018

Seems to be a Rust2018 related bug: rust-lang/rust#55344

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 25, 2018

@mati865 so i've merged a bunch of fixes in #181 to get CI and the builds green again

@mati865
Copy link
Contributor

mati865 commented Oct 25, 2018

Nice, as I said in my previous comment tests still fail (with NaN).

@mati865
Copy link
Contributor

mati865 commented Feb 8, 2019

I tested it again recently and it still happens for debug builds (release is fine) using packed_simd.

Failing example extracted from testsuite:

use packed_simd::f32x2;

fn main() {
    let z = f32x2::splat(0 as f32);
    let o = f32x2::splat(1 as f32);
    let t = f32x2::splat(2 as f32);
    assert_eq!(o, o.powf(z));
    assert_eq!(o, t.powf(z));
    assert_eq!(o, o.powf(o));
    assert_eq!(t, t.powf(o));

    let f = f32x2::splat(4 as f32);
    assert_eq!(f, t.powf(t));
}

assert_eq!(t, t.powf(o)) is going to fail because t.powf(o) returns f32x2(NaN, 2.0). Interesting things happen when we move this assert up (making it the first assert). It passes and now assert_eq!(f, t.powf(t)) fails with f32x2(NaN, 4.0).

IR and assembly at the bottom of this comment.


It does not reproduce when calling llvm intrinsics directly:

#![feature(
    repr_simd,
    simd_ffi,
    link_llvm_intrinsics
)]

#[repr(simd)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[allow(non_camel_case_types)]
struct f32x2(f32, f32);

#[allow(improper_ctypes)]
extern "C" {
    #[link_name = "llvm.pow.v2f32"]
    fn powf_v2f32(x: f32x2, y: f32x2) -> f32x2;
}

fn main() {
    let z = f32x2(0.0, 0.0);
    let o = f32x2(1.0, 1.0);
    let t = f32x2(2.0, 2.0);

    unsafe {
        assert_eq!(powf_v2f32(o, z), o);
        assert_eq!(powf_v2f32(t, z), o);
        assert_eq!(powf_v2f32(o, o), o);
        assert_eq!(powf_v2f32(t, o), t);
    }

    let f = f32x2(4.0, 4.0);
    unsafe {
        assert_eq!(powf_v2f32(t, t), f);
    }
}

For the failing example:
Fancy assembly from cargo-asm: https://gist.github.com/mati865/de756d37e395117b833959fcb4bbc16b#file-cargo-asm-s
Assembly emitted by rustc: https://gist.github.com/mati865/de756d37e395117b833959fcb4bbc16b#file-emit-asm-s
LLVM IR: https://gist.github.com/mati865/de756d37e395117b833959fcb4bbc16b#file-emit-ir-ll

Let me know if I can do something else to help here.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 8, 2019

What's the difference in the LLVM IR generated when calling intrinsics directly vs when using packed_simd ? Maybe that would reveal the problem.

@mati865
Copy link
Contributor

mati865 commented Feb 8, 2019

I'm pretty much beginner at this stuff but by my understanding outstanding thing here is alignment. Direct code operates on 8-byte alignment and packed_simd uses 4-byte alignment.

Direct IR: https://gist.github.com/mati865/7ff6c9f76284d1526d947d0f9d82130e#file-direct-ll
packed_simd IR: https://gist.github.com/mati865/7ff6c9f76284d1526d947d0f9d82130e#file-packed_simd-ll

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 8, 2019

@mati865 you can use -C debuginfo=0 to "prune" all the debug information from the IR - for some reason the Direct IR appears to contain packed_simd stuff.

Direct code operates on 8-byte alignment and packed_simd uses 4-byte alignment.

This might be it. I'll have to investigate why the f32x2 packed_simd type has incorrect alignment on windows.

Which target are you using ?

@mati865
Copy link
Contributor

mati865 commented Feb 8, 2019

@mati865 you can use -C debuginfo=0 to "prune" all the debug information from the IR - for some reason the Direct IR appears to contain packed_simd stuff.

I was actually wondering if there is a way to remove redundant data.

Direct: https://gist.github.com/mati865/4f0228869f70b2f677dd402ab9a9c6d5#file-direct-ll
packed_simd: https://gist.github.com/mati865/4f0228869f70b2f677dd402ab9a9c6d5#file-packed_simd-ll

Which target are you using ?

Thought I mentioned it but probably I removed it during pre-post edits.

At first tested it on 64 bit Windows 10 with native x86_64-pc-windows-gnu and later moved to my main 64 bit Linux environment. Cross-compiling to x86_64-pc-windows-gnu and testing executables with Wine (4.0 Staging) which shows exactly the same issue.
The build command I'm using: CARGO_INCREMENTAL=0 cargo rustc --target x86_64-pc-windows-gnu -- --emit llvm-ir -C debuginfo=0.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 11, 2019

So I haven't been able to reproduce this yet. Can you replace the asserts with a macro like this:

macro_rules! assert_eq_ {
    ($a:id, $b:id) => { if $a != $b { panic!() } }
}

? And remove the Debug impls, etc.

That would remove all the formatting code and hopefully leave only what matters (compile with -C debuginfo=0 -C panic=abort etc. ). It would also be interesting to know if it only reproduces with -C opt-level=0 or if it also reproduces with -C opt-level=1.

@mati865
Copy link
Contributor

mati865 commented Feb 11, 2019

Source code:

use packed_simd::f32x2;

macro_rules! assert_eq_ {
    ($a:expr, $b:expr) => { if $a != $b { panic!() } }
}

fn main() {
    let z = f32x2::splat(0.0);
    let o = f32x2::splat(1.0);
    let t = f32x2::splat(2.0);
    assert_eq_!(o, o.powf(z));
    assert_eq_!(o, t.powf(z));
    assert_eq_!(o, o.powf(o));
    assert_eq_!(t, t.powf(o));

    let f = f32x2::splat(4 as f32);
    assert_eq_!(f, t.powf(t));
}

Build command: CARGO_INCREMENTAL=0 cargo rustc --target x86_64-pc-windows-gnu -- --emit llvm-ir -C debuginfo=0 -C panic=abort -C opt-level=0.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 11, 2019

Cool, that means that the issue persist with O1 and packed_simd, could you provide the O1 code with the same compilation options without packed_simd ? the diff should reveal the issue.

@mati865
Copy link
Contributor

mati865 commented Feb 11, 2019

#![feature(
    repr_simd,
    simd_ffi,
    link_llvm_intrinsics
)]

#[repr(simd)]
#[derive(Copy, Clone, PartialEq)]
#[allow(non_camel_case_types)]
struct f32x2(f32, f32);

#[allow(improper_ctypes)]
extern "C" {
    #[link_name = "llvm.pow.v2f32"]
    fn powf_v2f32(x: f32x2, y: f32x2) -> f32x2;
}

macro_rules! assert_eq_ {
    ($a:expr, $b:expr) => { if $a != $b { panic!() } }
}

fn main() {
    let z = f32x2(0.0, 0.0);
    let o = f32x2(1.0, 1.0);
    let t = f32x2(2.0, 2.0);

    unsafe {
        assert_eq_!(powf_v2f32(o, z), o);
        assert_eq_!(powf_v2f32(t, z), o);
        assert_eq_!(powf_v2f32(o, o), o);
        assert_eq_!(powf_v2f32(t, o), t);
    }

    let f = f32x2(4.0, 4.0);
    unsafe {
        assert_eq_!(powf_v2f32(t, t), f);
    }
}

Built with CARGO_INCREMENTAL=0 cargo rustc --target x86_64-pc-windows-gnu -- --emit llvm-ir -C debuginfo=0 -C panic=abort -C opt-level=1.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 11, 2019

There is still too much noise. If you are sure that the O1 version of non-packed-simd passes without an assert, and the O1 version of packed-simd fails with an assert, can you compile these as libraries (crate_type = "lib") instead of as a binary (ideally using #[no_std] to also remove allocator-related noise)? That might remove even more noise.

@mati865
Copy link
Contributor

mati865 commented Feb 11, 2019

no_std libs:

I must have been sleepy last time I checked assembly, alignment difference exists because direct code calls <2 x float> @llvm.pow.v2f32(<2 x float>, <2 x float>) and packed_simd calls float @llvm.pow.f32(float, float).

@mati865
Copy link
Contributor

mati865 commented Mar 10, 2019

With a lot of cargo-expand, regex and C-Reduce I was able to make smaller example that triggers the bug:
https://gist.github.com/mati865/7f8e6600ec15f509afca8f316e7d4be3

Running:

rustc code.rs --target x86_64-pc-windows-gnu -C linker=x86_64-w64-mingw32-gcc >/dev/null 2>&1 &&\
! wine64 code.exe

@mati865
Copy link
Contributor

mati865 commented Mar 10, 2019

Reduced it further and updated the gist: https://gist.github.com/mati865/7f8e6600ec15f509afca8f316e7d4be3.

Generated IR is quite large and I don't see important differences between linux and windows-gnu.
Assembly is shorter and although I don't know it well windows-gnu looks weird to me when compared to linux: https://godbolt.org/z/bqZn4-

I'll also post that in Rust issue.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 11, 2019

https://godbolt.org/z/-AF3xb uses -C debuginfo=0 -C panic=abort --emit=llvm-ir and they both look really similar to me.

@mati865
Copy link
Contributor

mati865 commented Mar 11, 2019

Oops, I meant assembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked-Rust Issues blocked on rust-lang/rust CI-Appveyor Appveyor CI O-Windows P-Medium Unsound Something breaks Rust safety guarantees
Projects
None yet
Development

No branches or pull requests

3 participants