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

Array bounds check not elided when indexing with ZST enum #82871

Closed
moxian opened this issue Mar 7, 2021 · 3 comments · Fixed by #83020
Closed

Array bounds check not elided when indexing with ZST enum #82871

moxian opened this issue Mar 7, 2021 · 3 comments · Fixed by #83020
Assignees
Labels
C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@moxian
Copy link
Contributor

moxian commented Mar 7, 2021

I tried this code:

#[repr(C)]
pub enum E {
    A, 
}

pub fn index(x: &[u32; 3], ind: E) -> u32{
    x[ind as usize]
}

I expected to see this happen: no bounds check

Instead, this happened (with -C opt-level=3)

example::index:
        push    rax
        mov     rax, rdi
        mov     edi, esi
        cmp     esi, 2
        ja      .LBB0_2
        mov     eax, dword ptr [rax + 4*rdi]
        pop     rcx
        ret
.LBB0_2:
        lea     rdx, [rip + .L__unnamed_1]
        mov     esi, 3
        call    qword ptr [rip + core::panicking::panic_bounds_check@GOTPCREL]
        ud2

However adding more variants to the enum makes the bound check go away:

#[repr(C)]
pub enum E {
    A,
    B,
}

pub fn index(x: &[u32; 3], ind: E) -> u32{
    x[ind as usize]
}

generates:

example::index:
        mov     eax, esi
        mov     eax, dword ptr [rdi + 4*rax]
        ret

Here's a godbolt link to play around further: https://rust.godbolt.org/z/dK9e1z
The MIR of the two cases is identical, but the llvm IR in ZST case is lacking @llvm.assume, which I assume (heh!) leads to the bounds check being preserved. (However i don't actually understand LLVM, I'm just reading the diff...)

Meta

rustc 1.50.0

This sounds very closely related to #13926, but I'm not actually sure if it is. Please reroute as appropriate.

@moxian moxian added the C-bug Category: This is a bug. label Mar 7, 2021
@elomatreb
Copy link
Contributor

Removing any #[repr(...)] attributes from the enum also leads to the optimization occurring as expected (https://rust.godbolt.org/z/fPn613).

@nikic
Copy link
Contributor

nikic commented Mar 8, 2021

We don't emit the enum range assumption if the range only contains one element:

&& scalar.valid_range.end() > scalar.valid_range.start()

@estebank estebank added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2021
@nikic nikic added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 9, 2021
@Rustin170506
Copy link
Member

@rustbot claim

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 12, 2021
Emit the enum range assumption if the range only contains one element

close rust-lang#82871
@bors bors closed this as completed in 04e24ae Mar 13, 2021
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. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants