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

Codegen and const eval evaluates -1.0 % -1.0 to 0 with different signs #109567

Closed
cbeuw opened this issue Mar 24, 2023 · 8 comments
Closed

Codegen and const eval evaluates -1.0 % -1.0 to 0 with different signs #109567

cbeuw opened this issue Mar 24, 2023 · 8 comments
Assignees
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cbeuw
Copy link
Contributor

cbeuw commented Mar 24, 2023

pub fn f() -> f64 {
    std::hint::black_box(-1.0) % std::hint::black_box(-1.0)
}

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

pub fn main() {
    println!("with black box: {}", f());
    println!("without black box: {}", g());
    assert_eq!(f().signum(), g().signum());
}
% rustc repro.rs && ./repro
with black box: -0
without black box: 0
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `-1.0`,
 right: `1.0`', repro.rs:12:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Not sure whether -1.0 % -1.0 should be 0 or -0 according to IEEE 754, but codegen (with black box) and const eval (without blackbox) should produce the same result

Meta

rustc --version --verbose:

rustc 1.70.0-nightly (1459b3128 2023-03-23)
binary: rustc
commit-hash: 1459b3128e288a85fcc4dd1fee7ada2cdcf28794
commit-date: 2023-03-23
host: aarch64-apple-darwin
release: 1.70.0-nightly
LLVM version: 15.0.7
@cbeuw cbeuw added the C-bug Category: This is a bug. label Mar 24, 2023
@cbeuw
Copy link
Contributor Author

cbeuw commented Mar 24, 2023

IEEE 754-2019 §5.3.1 says

When y ≠ 0, the remainder r = remainder(x, y) is defined for finite x and y [...] If r = 0, its sign shall be that of x. [...]

So -1.0 % -1.0 should be -0.0, so codegen is correct.

Looking at const eval code, this is handled correctly, so the sign somehow got dropped later on? (nvm this function isn't used anywhere)

if v.is_zero() {
status.and(v.copy_sign(self)) // IEEE754 requires this
} else {
status.and(v)
}

@chenyukang
Copy link
Member

I think it's optimized, https://rust.godbolt.org/z/Eqae9xPKE

example::f:
        sub     rsp, 24
        mov     rax, qword ptr [rip + core::hint::black_box@GOTPCREL]
        mov     qword ptr [rsp], rax
        movsd   xmm0, qword ptr [rip + .LCPI7_0]
        movsd   qword ptr [rsp + 8], xmm0
        call    rax
        mov     rax, qword ptr [rsp]
        movaps  xmm1, xmm0
        movsd   xmm0, qword ptr [rsp + 8]
        movsd   qword ptr [rsp + 16], xmm1
        call    rax
        movaps  xmm1, xmm0
        movsd   xmm0, qword ptr [rsp + 16]
        mov     rax, qword ptr [rip + fmod@GOTPCREL]
        call    rax
        add     rsp, 24
        ret

example::g:
        xorps   xmm0, xmm0
        ret

@cbeuw
Copy link
Contributor Author

cbeuw commented Mar 24, 2023

it's optimized

Not by LLVM. g is already 0 in MIR

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn f() -> f64 {
    let mut _0: f64;                     // return place in scope 0 at repro.rs:1:15: 1:18
    let mut _1: f64;                     // in scope 0 at repro.rs:2:5: 2:31
    let mut _2: f64;                     // in scope 0 at repro.rs:2:34: 2:60

    bb0: {
        _1 = std::hint::black_box::<f64>(const -1f64) -> bb1; // scope 0 at repro.rs:2:5: 2:31
                                         // mir::Constant
                                         // + span: repro.rs:2:5: 2:25
                                         // + literal: Const { ty: fn(f64) -> f64 {std::hint::black_box::<f64>}, val: Value(<ZST>) }
    }

    bb1: {
        _2 = std::hint::black_box::<f64>(const -1f64) -> bb2; // scope 0 at repro.rs:2:34: 2:60
                                         // mir::Constant
                                         // + span: repro.rs:2:34: 2:54
                                         // + literal: Const { ty: fn(f64) -> f64 {std::hint::black_box::<f64>}, val: Value(<ZST>) }
    }

    bb2: {
        _0 = Rem(move _1, move _2);      // scope 0 at repro.rs:2:5: 2:60
        return;                          // scope 0 at repro.rs:3:2: 3:2
    }
}

fn g() -> f64 {
    let mut _0: f64;                     // return place in scope 0 at repro.rs:5:15: 5:18

    bb0: {
        _0 = const 0f64;                 // scope 0 at repro.rs:6:5: 6:16
        return;                          // scope 0 at repro.rs:7:2: 7:2
    }
}

@chenyukang
Copy link
Member

it's done by mir-opt, if we disable it the result will be same:

rustc +dev --edition 2021 ./p/debug3.rs -Z mir-opt-level=0 

I'm still investigating the specific line of code, but it seems in part of compiler/rustc_mir_transform.

@chenyukang
Copy link
Member

The line of code is here:

self.binary_float_op(bin_op, ty, left.to_f64()?, right.to_f64()?)

@RalfJung
Copy link
Member

This looks to me like a duplicate of #102403?

@cbeuw
Copy link
Contributor Author

cbeuw commented Mar 25, 2023

This looks to me like a duplicate of #102403?

Yes it's the same root cause. Although the reproduction here requires only stable rust

@jyn514 jyn514 added A-const-eval Area: Constant evaluation (MIR interpretation) A-floating-point Area: Floating point numbers and arithmetic labels Mar 27, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2023

Closing as a duplicate of #102403, then.

@RalfJung RalfJung closed this as completed Jul 6, 2023
wesleywiser pushed a commit to wesleywiser/rust that referenced this issue Jul 26, 2023
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 (MIR interpretation) A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants