Skip to content

Cherry-pick of upstreamed LLVM patch that fix MIPS int shift miscompilation #156

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

Conversation

martn3
Copy link

@martn3 martn3 commented Nov 8, 2023

Summary

This PR is a cherry-pick of an LLVM upstream patch that fixes the MIPS miscompilation reported in rust-lang/rust#116177.

Details

The Rust git master branch miscompiles the following program for --target mips[el]-unknown-linux-gnu. The program comes from the above Rust issue that reported the problem.

main.rs

use std::hint::black_box;

const SHIFTVALUE : i64 = 10000000000i64;

fn main() {

    fn shiftleft(a : i64, b : i64) -> i64 {
        a << b
    }

    assert_eq!(shiftleft(black_box(SHIFTVALUE), black_box(2)), SHIFTVALUE << 2);
}

How to reproduce

To reproduce the miscompilation for MIPS with qemu on a Debian 12 x86_64 host with the Rust git master branch, do the following:

sudo apt install qemu-user gcc-mips-linux-gnu

./x build \
    --set llvm.download-ci-llvm=false \
    --set llvm.targets="Mips;X86" \
    --target mips-unknown-linux-gnu

./build/x86_64-unknown-linux-gnu/stage1/bin/rustc \
    --target mips-unknown-linux-gnu \
    -Clinker=mips-linux-gnu-gcc \
    -Ctarget-feature=+crt-static \
    main.rs

qemu-mips ./main

The assert should pass, but it fails with:

thread 'main' panicked at main.rs:11:5:
assertion `left == right` failed
  left: 35705032704
 right: 40000000000
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

How to fix

The miscompilation was fixed by the LLVM PR llvm#71149. This PR cherry-picks that commit. (The upstream LLVM issue that was fixed is llvm#71142. I believe it also fixed the LLVM issue llvm#64794 even though it is still open.)

I have confirmed that the Rust test program pass after applying the cherry-picked patch to the Rust LLVM fork and building with Rust git master.

If we start with an i128 shift, the initial shift amount would usually
have zeros in bit 8 and above. xoring the shift amount with -1 will set
those upper bits to 1. If DAGCombiner is able to prove those bits are
now 1, then the shift that uses the xor will be replaced with undef.
Which we don't want.

Reduce the xor constant to VT.bits-1 where VT is half the size of the
larger shift type. This avoids toggling the upper bits. The hardware
shift instruction only uses the lower bits of the shift amount. I assume
the code used NOT because the hardware doesn't use the upper bits, but
that isn't compatible with the LLVM poison semantics.

Fixes llvm#71142.

(cherry picked from commit
llvm@8d24d39)
@nikic
Copy link

nikic commented Nov 8, 2023

This needs an upstream backport first, already requested on llvm#64794.

@nikic nikic closed this Nov 8, 2023
vext01 pushed a commit to vext01/llvm-project that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants