Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Unneeded call to panic!() #73031

Closed
jrmuizel opened this issue Jun 5, 2020 · 5 comments
Closed

Unneeded call to panic!() #73031

jrmuizel opened this issue Jun 5, 2020 · 5 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

@jrmuizel
Copy link
Contributor

jrmuizel commented Jun 5, 2020

The following:

pub enum All {
    None,
    Foo,
    Bar,
}

pub fn f(a: &mut All, q: i32) -> i32 {
    *a = if (q == 5) {
        All::Foo
    } else {
        All::Bar
    };
    match *a {
        All::None => unreachable!(),
        All::Foo => 1,
        All::Bar => 2,
    }
}

compiles to:

example::f:
        cmp     esi, 5
        sete    al
        mov     cl, 2
        sub     cl, al
        mov     byte ptr [rdi], cl
        mov     eax, 1
        cmp     cl, 1
        je      .LBB6_3
        cmp     cl, 2
        jne     .LBB6_4
        mov     eax, 2
.LBB6_3:
        ret
.LBB6_4:
        push    rax
        call    std::panicking::begin_panic
        ud2

The unreachable! and should be eliminated

@jrmuizel jrmuizel added the C-bug Category: This is a bug. label Jun 5, 2020
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. 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. and removed C-bug Category: This is a bug. labels Jun 5, 2020
@erikdesjardins
Copy link
Contributor

erikdesjardins commented Jun 8, 2020

This optimizes well with u8 instead of an enum:

u8
pub fn foo(a: &mut u8, q: i32) -> i32 {
    *a = if q == 5 {
        1
    } else {
        2
    };
    match *a {
        1 => 1,
        2 => 2,
        _ => unreachable!(),
    }
}
example::foo:
        xor     ecx, ecx
        cmp     esi, 5
        sete    cl
        mov     al, 2
        sub     al, cl
        mov     byte ptr [rdi], al
        mov     eax, 2
        sub     eax, ecx
        ret

With an enum, the important part of the optimized LLVM IR looks like this:

start:
  %_4 = icmp eq i32 %q, 5
  %. = select i1 %_4, i8 1, i8 2
  store i8 %., i8* %a, align 1
  %_6 = zext i8 %. to i64
  switch i64 %_6, label %bb5 [
    i64 0, label %bb4
    i64 1, label %bb8
    i64 2, label %bb7
  ]

And with u8:

start:
  %_4 = icmp eq i32 %q, 5
  %. = select i1 %_4, i8 1, i8 2
  store i8 %., i8* %a, align 1
  %spec.select = select i1 %_4, i32 1, i32 2
  ret i32 %spec.select

So it seems that LLVM doesn't propagate value ranges though zext (or something, I don't know LLVM internals). And sure enough, if I add as u64 to the u8 code, it generates a panic branch:

u8 as u64
pub fn foo(a: &mut u8, q: i32) -> i32 {
    *a = if q == 5 {
        1
    } else {
        2
    };
    match *a as u64 {
        1 => 1,
        2 => 2,
        _ => unreachable!(),
    }
}
example::foo:
        push    rax
        xor     r8d, r8d
        cmp     esi, 5
        sete    dl
        mov     cl, 2
        sub     cl, dl
        mov     byte ptr [rdi], cl
        mov     eax, 1
        cmp     cl, 1
        je      .LBB5_3
        mov     r8b, dl
        mov     eax, 2
        sub     rax, r8
        cmp     rax, 2
        jne     .LBB5_4
        mov     eax, 2
.LBB5_3:
        pop     rcx
        ret
.LBB5_4:
        call    std::panicking::begin_panic
        ud2

With an enum, various things that make the size of the discriminant the same as the size of the enum in memory (hence avoiding the zext) allow LLVM to remove the panic branch, e.g. a repr hint or an explicit discriminant that's different (?) from what the implicit discriminant would be:

enums
// this generates a panic branch
pub enum All {
    None = 0,
    Foo = 1,
    Bar = 2,
}

// these do not
#[repr(u8)]
pub enum All {
    None,
    Foo,
    Bar,
}
pub enum All {
    None = 1,
    Foo = 2,
    Bar = 3,
}
pub enum All {
    None,
    Foo,
    Bar = 3,
}
pub enum All {
    None = -1,
    Foo,
    Bar,
}

I don't know why rustc does this. I assume it has something to do with the default discriminant type being isize.

Edit: it seems that LLVM is able to see through the zext in...some cases. I can't tell why, and it doesn't really seem to help us. See: https://godbolt.org/z/W8SeHC.

@jrmuizel
Copy link
Contributor Author

jrmuizel commented Jun 8, 2020

I've filed the upstream issue: https://bugs.llvm.org/show_bug.cgi?id=46244

@eddyb is it possible for us to avoid the zext when generating code for this example?

@erikdesjardins
Copy link
Contributor

@rustbot modify labels: +A-LLVM

@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jul 14, 2020
@erikdesjardins
Copy link
Contributor

Like #77812 (comment), this will be fixed in LLVM 12: https://llvm.godbolt.org/z/jxjEbq

@MSxDOS
Copy link

MSxDOS commented Mar 5, 2021

Seems to be fixed on the latest nightly by #81451

@nikic nikic closed this as completed Mar 6, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 8, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

No branches or pull requests

6 participants