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

u128 comparison fails with opt-level 1 #1484

Closed
cuviper opened this issue Apr 20, 2024 · 3 comments · Fixed by bytecodealliance/wasmtime#8422
Closed

u128 comparison fails with opt-level 1 #1484

cuviper opened this issue Apr 20, 2024 · 3 comments · Fixed by bytecodealliance/wasmtime#8422
Labels
C-bug Category: This is a bug. upstream Caused by a dependency

Comments

@cuviper
Copy link
Member

cuviper commented Apr 20, 2024

This little program is fine with opt 0, but fails with opt 1.

fn main() {
    const X: u128 = 0x8000_0000_0000_0000;
    assert_eq!((|| X)(), X);
}
$ rustc +nightly foo.rs -Zcodegen-backend=cranelift -Copt-level=1 && ./foo
thread 'main' panicked at foo.rs:3:5:
assertion `left == right` failed
  left: 9223372036854775808
 right: 9223372036854775808
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Note that this constant could fit in u64, but it looks like it's sign-extending that %r8 MSB into %rcx:

    845c:       e8 86 00 00 00          call   84e7 <_ZN3foo4main28_$u7b$$u7b$closure$u7d$$u7d$17h8cd7c7807c97b1efE>
    8461:       48 8d 34 24             lea    (%rsp),%rsi
    8465:       48 89 06                mov    %rax,(%rsi)
    8468:       48 89 56 08             mov    %rdx,0x8(%rsi)
    846c:       48 8d 34 24             lea    (%rsp),%rsi
    8470:       48 8b 06                mov    (%rsi),%rax
    8473:       48 8b 56 08             mov    0x8(%rsi),%rdx
    8477:       49 b8 00 00 00 00 00    movabs $0x8000000000000000,%r8
    847e:       00 00 80
    8481:       4c 89 c1                mov    %r8,%rcx
    8484:       48 c1 f9 3f             sar    $0x3f,%rcx
    8488:       4c 39 c0                cmp    %r8,%rax
    848b:       41 0f 94 c0             sete   %r8b
    848f:       48 39 ca                cmp    %rcx,%rdx
    8492:       41 0f 94 c1             sete   %r9b
    8496:       4d 21 c8                and    %r9,%r8
    8499:       49 f7 c0 01 00 00 00    test   $0x1,%r8
    84a0:       0f 84 09 00 00 00       je     84af <_ZN3foo4main17hb0af6480194903a8E+0x62>
    84a6:       48 83 c4 50             add    $0x50,%rsp
    84aa:       48 89 ec                mov    %rbp,%rsp
    84ad:       5d                      pop    %rbp
    84ae:       c3                      ret

The closure properly zeros the top half instead:

00000000000084e7 <_ZN3foo4main28_$u7b$$u7b$closure$u7d$$u7d$17h8cd7c7807c97b1efE>:
    84e7:       55                      push   %rbp
    84e8:       48 89 e5                mov    %rsp,%rbp
    84eb:       48 b8 00 00 00 00 00    movabs $0x8000000000000000,%rax
    84f2:       00 00 80
    84f5:       48 31 d2                xor    %rdx,%rdx
    84f8:       48 89 ec                mov    %rbp,%rsp
    84fb:       5d                      pop    %rbp
    84fc:       c3                      ret

$ rustc +nightly -Vv
rustc 1.79.0-nightly (f9b161492 2024-04-19)
binary: rustc
commit-hash: f9b16149208c8a8a349c32813312716f6603eb6f
commit-date: 2024-04-19
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.4
@bjorn3
Copy link
Member

bjorn3 commented Apr 20, 2024

-Zmir-opt-level=2 without -Copt-level=1 reproduces this too. Looks like

v3 = icmp_imm eq v2, 0x8000_0000_0000_0000

gets legalized to

v12 = iconst.i64 0x8000_0000_0000_0000
v13 = sextend.i128 v12  ; v12 = 0x8000_0000_0000_0000
v3 = icmp eq v2, v13

which is to be expected as immediates in clif ir are signed. The problem is that cranelift_frontend::switch::icmp_imm_u128 uses icmp_imm here despite the value to check against not fitting in an i64.

@bjorn3
Copy link
Member

bjorn3 commented Apr 20, 2024

bytecodealliance/wasmtime#8422 fixes this bug.

@bjorn3 bjorn3 added C-bug Category: This is a bug. upstream Caused by a dependency labels Apr 20, 2024
@bjorn3
Copy link
Member

bjorn3 commented May 30, 2024

Fixed with the latest Cranelift release.

@bjorn3 bjorn3 closed this as completed May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. upstream Caused by a dependency
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants