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

Sign of zero from differs between CTFE and normal execution (float % float) #102403

Closed
jruderman opened this issue Sep 28, 2022 · 10 comments · Fixed by rust-lang/rustc_apfloat#1 or #113843
Closed
Assignees
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-floating-point Area: Floating point numbers and arithmetic A-mir-opt Area: MIR optimizations C-bug Category: This is a bug.

Comments

@jruderman
Copy link
Contributor

With this code:

#![feature(const_fn_floating_point_arithmetic)]

const fn f(one: f64) -> f64 {
    (-1.0) % one
}

const RESULT_CT : f64 = f(1.0);

fn main() {
    let black_box_one = (std::env::args().len()) as f64;
    let result_rt = f(black_box_one);
    assert_eq!(RESULT_CT.is_sign_negative(), result_rt.is_sign_negative());
}

The assertion fails: compile-time evaluation gives 0.0 while runtime evaluation gives -0.0.

This is probably relevant to tracking issue #57241.

@jruderman jruderman added the C-bug Category: This is a bug. label Sep 28, 2022
@scottmcm
Copy link
Member

Reminds me of #55131, though this one's probably less ugly since signs of zeros are less FUBAR than signs of NANs.

@bjorn3 bjorn3 added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Oct 4, 2022
@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2022

Probably another apfloat bug, either in the LLVM original or in the transpilation?
Which is even the correct answer?
Cc @eddyb @workingjubilee
Cc rust-lang/unsafe-code-guidelines#237

@thomcc thomcc added the A-floating-point Area: Floating point numbers and arithmetic label Oct 9, 2022
@eddyb
Copy link
Member

eddyb commented Oct 10, 2022

To look at LLVM, just feed it through constant folding: https://godbolt.org/z/oWqrhKe4h

So it looks like current LLVM APFloat results in negative sign.
This remains the case even when going back to 1.20 (which should be old enough to reflect the state of LLVM at the time of the porting): https://godbolt.org/z/8dj73sEjr.

So this looks like it might be another bug like #100233 (comment) :/


EDIT: you don't need to run any code to see the bug in action, you can trigger it with MIR inlining: https://godbolt.org/z/nYxchqv9P

Current output (just the relevant parts):

fn f_one() -> f64 {
    let mut _0: f64;
    bb0: {
        _0 = const 0f64;
        return;
    }
}

fn minus_zero() -> f64 {
    let mut _0: f64;
    bb0: {
        _0 = const -0f64;
        return;
    }
}

@oli-obk oli-obk self-assigned this Nov 5, 2022
@oli-obk oli-obk added the A-mir-opt Area: MIR optimizations label Nov 5, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Nov 5, 2022

So... basically const prop is unsound around floats?

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2022

The example is a CTFE bug, not a ConstProp bug.
Do we even do ConstProp on floats ourselves?

@saethlin
Copy link
Member

Using the reproducer from #109567

Do we even do ConstProp on floats ourselves?

Yes. ConstProp disagrees with LLVM: https://godbolt.org/z/5dPrfqMYq

(once again wishing we could either pin to a nightly in godbolt or otherwise set mir-opt flags in a stable compiler, because this link will eventually not reproduce if we fix this)

pub fn make_me_a_float() -> f64 {
    -1.0 % -1.0
}

With --emit=mir we get:

fn make_me_a_float() -> f64 {
    let mut _0: f64;                     // return place in scope 0 at /app/example.rs:1:29: 1:32

    bb0: {
        _0 = const 0f64;                 // scope 0 at /app/example.rs:2:5: 2:16
        return;                          // scope 0 at /app/example.rs:3:2: 3:2
    }
}

But with --emit=llvm-ir -Zmir-enable-passes=-ConstProp we get:

define double @_ZN7example15make_me_a_float17h53a3e1048ece245eE() unnamed_addr #0 !dbg !6 {
  ret double -0.000000e+00, !dbg !11
}

@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2023

#109567 has some other examples reproducing this.

@eddyb
Copy link
Member

eddyb commented Jul 11, 2023

While trying to make progress on #113409 I noticed this was showing up as "C++ llvm::APFloat and Rust rustc_apfloat agree, but hardware disagrees", which I was confused by because I've shown above (#102403 (comment)) rustc 1.20 constant-folds it correctly in LLVM (and even rustc 1.0 does AFAICT).

But, this is not a porting bug, it's a LLVM one, fixed very soon after my port: llvm/llvm-project@f2c2851

My best guess is that LLVM was unaffected by its own bug because the host C fmod function was used instead (a LLVM commit from 2 years later, llvm/llvm-project@ed5f452 might imply something like that, but I haven't 100% confirmed that's what's going on).


EDIT: for the record, I would not have figured this out as easily if not for my WIP fuzzing system:
image
(turns out instrumentation-based fuzzers and softfloat impls are a great match, I'm guessing because of how all the special cases are handled with branches, and depend on relatively simple bitpatterns of the input)

@eddyb
Copy link
Member

eddyb commented Jul 13, 2023

I posted a longer update to #55993 (comment) regarding the progress with the whole rustc_apfloat situation, but of note for this issue is that the WIP repo already has this bug fixed.

@eddyb
Copy link
Member

eddyb commented Jul 18, 2023

Accidentally closed (overzealous GitHub), my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-floating-point Area: Floating point numbers and arithmetic A-mir-opt Area: MIR optimizations C-bug Category: This is a bug.
Projects
None yet
9 participants