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

incorrect code generation for i686 release build for 1.47 beta and nightly #76042

Closed
tspiteri opened this issue Aug 28, 2020 · 12 comments
Closed
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections O-x86_32 Target: x86 processors, 32 bit (like i686-*) P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tspiteri
Copy link
Contributor

/// ```rust
/// assert_eq!(fixed::i124f4_wrapping_mul(3 << 4, 2 << 4), 6 << 4);
/// ```
#[inline]
pub fn i124f4_wrapping_mul(lhs: i128, rhs: i128) -> i128 {
    let (ans, _) = mul_overflow(lhs, rhs, 4);
    ans
}

#[inline]
fn mul_overflow(lhs: i128, rhs: i128, frac_nbits: u32) -> (i128, bool) {
    let (lh, ll) = hi_lo(lhs);
    let (rh, rl) = hi_lo(rhs);
    let ll_rl = ll.wrapping_mul(rl);
    let lh_rl = lh.wrapping_mul(rl);
    let ll_rh = ll.wrapping_mul(rh);
    let lh_rh = lh.wrapping_mul(rh);

    let col01 = ll_rl as u128;
    let (col01_hi, col01_lo) = (col01 >> 64, col01 & !(!0 << 64));
    let partial_col12 = lh_rl + col01_hi as i128;
    let (col12, carry_col3) = carrying_add(partial_col12, ll_rh);
    let (col12_hi, col12_lo) = hi_lo(col12);
    let ans01 = shift_lo_up_unsigned(col12_lo) + col01_lo;
    let ans23 = lh_rh + col12_hi + shift_lo_up(carry_col3);
    let ret = combine_lo_then_shl(ans23, ans01, frac_nbits);
    println!(
        "combine_lo_then_shl({}, {}, {}) -> ({}, {})",
        ans23, ans01, frac_nbits, ret.0, ret.1
    );
    ret
}

#[inline]
fn hi_lo(this: i128) -> (i128, i128) {
    (this >> 64, this & !(!0 << 64))
}

#[inline]
fn combine_lo_then_shl(this: i128, lo: u128, shift: u32) -> (i128, bool) {
    if shift == 128 {
        (this, false)
    } else if shift == 0 {
        let ans = lo as i128;
        (ans, this != if ans < 0 { -1 } else { 0 })
    } else {
        let lo = (lo >> shift) as i128;
        let hi = this << (128 - shift);
        let ans = lo | hi;
        (ans, this >> shift != if ans < 0 { -1 } else { 0 })
    }
}

#[inline]
fn carrying_add(this: i128, rhs: i128) -> (i128, i128) {
    let (sum, overflow) = this.overflowing_add(rhs);
    let carry = if overflow {
        if sum < 0 {
            1
        } else {
            -1
        }
    } else {
        0
    };
    (sum, carry)
}

#[inline]
fn shift_lo_up(this: i128) -> i128 {
    debug_assert!(this >> 64 == 0);
    this << 64
}

#[inline]
fn shift_lo_up_unsigned(this: i128) -> u128 {
    debug_assert!(this >> 64 == 0);
    (this << 64) as u128
}

The test passes for 1.46.0 i686 stable release builds, for i686 beta/nightly debug builds, and for x86_64 beta/nightly release builds, but fails for i686 beta/nightly release builds.

CI logs from this code on GitLab:

https://gitlab.com/tspiteri/fixed/-/pipelines/183281142

@tspiteri tspiteri added the C-bug Category: This is a bug. label Aug 28, 2020
@jonas-schievink jonas-schievink added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 28, 2020
@tspiteri
Copy link
Contributor Author

The println! prints combine_lo_then_shl(0, 1536, 4) -> (1536, true), but combine_lo_then_shl(0, 1536, 4) should return (96, false).

@LeSeulArtichaut LeSeulArtichaut added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Aug 28, 2020
@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Aug 28, 2020

Let's try to find the culprit for this regression. Trying to reduce the example would be nice too.
@rustbot ping cleanup

@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2020

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @camelid @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

@rustbot rustbot added the ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections label Aug 28, 2020
@LeSeulArtichaut LeSeulArtichaut added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Aug 28, 2020
@Dylan-DPC-zz Dylan-DPC-zz added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 29, 2020
@Dylan-DPC-zz
Copy link

Marking this as P-critical based on the wg-prioritization discussion

@tspiteri
Copy link
Contributor Author

I have a reduced test case.

/// ```rust
/// assert_eq!(rust76042::foo(), 8);
/// ```
#[inline]
pub fn foo() -> i128 {
    bar().0
}

#[inline]
fn bar() -> (i128, bool) {
    let a = 0;
    let b = 128;
    let shift = 4;
    let ret = baz(a, b, shift);
    // should print "baz(0, 128, 4) -> (8, false)"
    println!("baz({}, {}, {}) -> ({}, {})", a, b, shift, ret.0, ret.1);
    ret
}

#[inline]
fn baz(a: i128, b: i128, shift: u32) -> (i128, bool) {
    if shift == 128 {
        (a, false)
    } else {
        (b >> shift, a >> shift != 0)
    }
}

Note that if I move the doc test to a lib test, I cannot trigger this bug. On i686 beta/nightly release test, the doc test fails with:

---- src/lib.rs - foo (line 1) stdout ----
Test executable failed (exit code 101).
stdout:
baz(0, 128, 4) -> (128, true)
stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `128`,
 right: `8`', src/lib.rs:4:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

baz is somehow returning (128, true) instead of (8, false).

https://gitlab.com/tspiteri/rust76042/-/pipelines/183379486

@tmiasko
Copy link
Contributor

tmiasko commented Aug 29, 2020

Minimized:

fn foo(a: i128, b: i128, s: u32) -> (i128, i128) {
    if s == 128 {
        (0, 0)
    } else {
        (b >> s, a >> s)
    }
}
fn main() {
    let r = foo(0, 8, 1);
    if r.0 != 4 {
        panic!();
    }
}
rustc --crate-type=bin  a.rs --target i686-unknown-linux-gnu -Coverflow-checks=off -Ccodegen-units=1 -Copt-level=1
./a
rustc --crate-type=bin  a.rs --target i686-unknown-linux-gnu -Coverflow-checks=off -Ccodegen-units=1 -Copt-level=0
./a
thread 'main' panicked at 'explicit panic', a.rs:11:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@tmiasko
Copy link
Contributor

tmiasko commented Aug 29, 2020

LLVM bug report https://bugs.llvm.org/show_bug.cgi?id=47278.

EDIT:

  • Introduced by upgrade to LLVM 11.
  • Triggers assert in fast register allocator used at opt-level=0:
Unexpected reg unit state
UNREACHABLE executed at /rustsrc/src/llvm-project/llvm/lib/CodeGen/RegAllocFast.cpp:485!

@cuviper cuviper added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-x86 labels Sep 2, 2020
@pnkfelix pnkfelix self-assigned this Sep 3, 2020
@tspiteri
Copy link
Contributor Author

LLVM bug 47278 has apparently been fixed in the LLVM tree and its 11.x branch. (I don't really know LLVM, I'm only mentioning this as a comment above mentioned that bug as a possible cause of this issue.)

@tspiteri
Copy link
Contributor Author

I cannot reproduce the failure with rustc 1.48.0-nightly (8b40853 2020-09-23) which includes #77063.

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Sep 24, 2020

Letting this issue open to track a beta backport of #77063.
Also, it might be good to have add a test in the suite for this issue. @tspiteri Would you mind filing a PR with your reproduction? Otherwise I can do it myself.

@tspiteri
Copy link
Contributor Author

@LeSeulArtichaut I don't know how to write this kind of test PR, please go ahead.

@tspiteri
Copy link
Contributor Author

Now the beta is working fine as well for me.

@LeSeulArtichaut LeSeulArtichaut removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Sep 30, 2020
@LeSeulArtichaut LeSeulArtichaut added P-medium Medium priority and removed P-critical Critical priority labels Sep 30, 2020
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 13, 2020
…r=pnkfelix

Add regression test for issue rust-lang#76042

Originally posted in rust-lang#76042 (comment).
r? `@pnkfelix`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 13, 2020
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#77151 (Add regression test for issue rust-lang#76042)
 - rust-lang#77996 (Doc change: Remove mention of `fnv` in HashMap)
 - rust-lang#78463 (Add type to `ConstKind::Placeholder`)
 - rust-lang#78984 (Rustdoc check option)
 - rust-lang#78985 (add dropck test for const params)
 - rust-lang#78996 (add explicit test for const param promotion)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@Nilstrieb Nilstrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections O-x86_32 Target: x86 processors, 32 bit (like i686-*) P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants