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

Bounds check elision fails when using cmp::max #113757

Closed
SUPERCILEX opened this issue Jul 16, 2023 · 4 comments · Fixed by #125347
Closed

Bounds check elision fails when using cmp::max #113757

SUPERCILEX opened this issue Jul 16, 2023 · 4 comments · Fixed by #125347
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@SUPERCILEX
Copy link
Contributor

SUPERCILEX commented Jul 16, 2023

I tried this code:

pub fn foo(v: &mut Vec<MaybeUninit<u8>>, size: usize)-> Option<&mut [MaybeUninit<u8>]> {
    if v.len() > max(1, size) {
        let start = v.len() - size;
        Some(&mut v[start..])
    } else {
        None
    }
}

Generates:

example::foo:
        push    rax
        mov     rcx, qword ptr [rdi + 16]
        test    rsi, rsi
        mov     eax, 1
        cmovne  rax, rsi
        cmp     rcx, rax
        jbe     .LBB0_1
        mov     rax, rcx
        sub     rax, rsi
        jb      .LBB0_5
        add     rax, qword ptr [rdi]
        jmp     .LBB0_4
.LBB0_1:
        xor     eax, eax
.LBB0_4:
        mov     rdx, rsi
        pop     rcx
        ret
.LBB0_5:
        lea     rdx, [rip + .L__unnamed_1]
        mov     rdi, rax
        mov     rsi, rcx
        call    qword ptr [rip + core::slice::index::slice_start_index_len_fail@GOTPCREL]
        ud2

.L__unnamed_2:
        .ascii  "/app/example.rs"

.L__unnamed_1:
        .quad   .L__unnamed_2
        .asciz  "\017\000\000\000\000\000\000\000\020\000\000\000\023\000\000"

But

pub fn foo(v: &mut Vec<MaybeUninit<u8>>, size: usize)-> Option<&mut [MaybeUninit<u8>]> {
    if v.len() > 1 && v.len() > size {
        let start = v.len() - size;
        Some(&mut v[start..])
    } else {
        None
    }
}

generates super clean assembly:

example::foo:
        mov     rdx, rsi
        mov     rcx, qword ptr [rdi + 16]
        mov     rax, rcx
        sub     rax, rsi
        add     rax, qword ptr [rdi]
        xor     esi, esi
        cmp     rcx, rdx
        cmovbe  rax, rsi
        cmp     rcx, 2
        cmovb   rax, rsi
        ret

Meta

rustc --version --verbose:

rustc 1.73.0-nightly (1065d876c 2023-07-09)
@SUPERCILEX SUPERCILEX added the C-bug Category: This is a bug. label Jul 16, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 16, 2023
@nikic
Copy link
Contributor

nikic commented Jul 16, 2023

Godbolt: https://rust.godbolt.org/z/bhb1zW9o6

@nikic nikic added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. C-bug Category: This is a bug. labels Jul 16, 2023
@nikic
Copy link
Contributor

nikic commented Jul 16, 2023

llvm/llvm-project#63896

dtcxzyw added a commit to llvm/llvm-project that referenced this issue Jul 24, 2023
Fixes #63896 and rust-lang/rust#113757.
This patch adds facts implied by llvm.smin/smax/umin/umax intrinsics.

Reviewed By: fhahn

Differential Revision: https://reviews.llvm.org/D155412
@dtcxzyw
Copy link

dtcxzyw commented Jul 24, 2023

It should be fixed by llvm/llvm-project@92a11eb.

@erikdesjardins
Copy link
Contributor

Fixed in beta/1.77: https://rust.godbolt.org/z/o618xE6fh

The LLVM 18 upgrade is in 1.78, so I'm not sure why.

@rustbot label +E-needs-test

@rustbot rustbot added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Mar 5, 2024
tesuji added a commit to tesuji/rustc that referenced this issue May 20, 2024
tesuji added a commit to tesuji/rustc that referenced this issue Jun 8, 2024
tesuji added a commit to tesuji/rustc that referenced this issue Jun 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 10, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 13, 2024
@bors bors closed this as completed in 7ac6c2f Jun 14, 2024
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Sep 7, 2024
Fixes llvm/llvm-project#63896 and rust-lang/rust#113757.
This patch adds facts implied by llvm.smin/smax/umin/umax intrinsics.

Reviewed By: fhahn

Differential Revision: https://reviews.llvm.org/D155412
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. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants