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 sign extension from i32 to i64 on AArch64 #21627

Closed
akosthekiss opened this issue Jan 25, 2015 · 2 comments · Fixed by #21671
Closed

Incorrect sign extension from i32 to i64 on AArch64 #21627

akosthekiss opened this issue Jan 25, 2015 · 2 comments · Fixed by #21671
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.

Comments

@akosthekiss
Copy link
Contributor

When compiling the following code (which is a minimized version of impl_to_primitive_int_to_int! from libcore/num/mod.rs) on AArch64 with rustc from 2015-01-24, the output is unfortunately NO :(

use std::num::Int;

fn main() {
    let n = 960 as i64;
    let min_value: i32 = Int::min_value();
    if min_value as i64 <= n {
        println!("OK :)");
    } else {
        println!("NO :(")
    }
}

The disassembled code of main starts as:

00000000000071ec <_ZN4main20h2d56baed33734ff2faaE>:
    71ec:       a9be6ffc        stp     x28, x27, [sp,#-32]!
    71f0:       a9017bfd        stp     x29, x30, [sp,#16]
    71f4:       910043fd        add     x29, sp, #0x10
    71f8:       d10443ff        sub     sp, sp, #0x110
    71fc:       b27a0fe8        orr     x8, xzr, #0x3c0
    7200:       f81e83a8        str     x8, [x29,#-24]
    7204:       94000048        bl      7324 <_ZN3num7i32.Int9min_value20hb9150aaae7f1b75agpbE>
    7208:       b81e43a0        str     w0, [x29,#-28]
    720c:       b89e43a8        ldrsw   x8, [x29,#-28]
    7210:       2a0803e0        mov     w0, w8
    7214:       2a0003e8        mov     w8, w0
    7218:       f85e83a9        ldr     x9, [x29,#-24]
    721c:       eb09011f        cmp     x8, x9

The problem is with the two moves at 0x7210 and 0x7214, which erroneously clear the top 32 bits of x8, which thus becomes 0x80000000 instead of the expected 0xffffffff80000000.

The error was first found in the rustup_20150109 branch of Servo (servo/servo#4716), but the latest Rust also exhibits the problem (as shown above).

@akosthekiss
Copy link
Contributor Author

Just had a chat on #llvm and got a lot of help from Tim Northover. He found out that the bug has already been discovered and fixed in LLVM: llvm-mirror/llvm@ca07e25

How/would it be possible to adapt that fix to the LLVM used by rust?

@akosthekiss
Copy link
Contributor Author

More thinking-aloud: since the problem is in FastISel, which can be turned off, the problem can be worked around at rust side with no need to touch the LLVM currently in use.

By passing some extra options (rustc -C llvm-args=-fast-isel=0), the generated code looks much better and the output becomes OK :) as expected. (At the cost of somewhat slower compilation, but I think that's a price worth to pay for correctness.)

00000000000071ec <_ZN4main20h74a595d6a5b2fbb6faaE>:
    71ec:       a9be6ffc        stp     x28, x27, [sp,#-32]!
    71f0:       a9017bfd        stp     x29, x30, [sp,#16]
    71f4:       910043fd        add     x29, sp, #0x10
    71f8:       d104c3ff        sub     sp, sp, #0x130
    71fc:       321a0fe8        orr     w8, wzr, #0x3c0
    7200:       2a0803e9        mov     w9, w8
    7204:       f81e83a9        str     x9, [x29,#-24]
    7208:       94000047        bl      7324 <_ZN3num7i32.Int9min_value20hb9150aaae7f1b75agpbE>
    720c:       b81e43a0        str     w0, [x29,#-28]
    7210:       b89e43a9        ldrsw   x9, [x29,#-28]
    7214:       f85e83aa        ldr     x10, [x29,#-24]
    7218:       eb0a0129        subs    x9, x9, x10

akosthekiss added a commit to akosthekiss/rust that referenced this issue Jan 26, 2015
Before ca07e256f62f772d14c42f41af46f2aeacc54983, LLVM's AArch64FastISel
had a sign (and zero?) extension bug. Until rustc gets its LLVM past
that commit, the way of workaround is to pass an option to LLVM that
forces the disabling of FastISel (which would normally kick in for -O0).

Fixes rust-lang#21627
@huonw huonw added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jan 26, 2015
akosthekiss added a commit to akosthekiss/rust that referenced this issue Jan 27, 2015
Before ca07e256f62f772d14c42f41af46f2aeacc54983, LLVM's AArch64FastISel
had a sign (and zero?) extension bug. Until rustc gets its LLVM past
that commit, the way of workaround is to pass an option to LLVM that
forces the disabling of FastISel (which would normally kick in for -O0).

Fixes rust-lang#21627
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants