Missed optimization with match on repr(i8) enum

[jrose-signal]

The following functions ought to be equivalent:

use core::cmp::Ordering;

pub fn test1(call: fn() -> Ordering) -> i32 {
    match call() {
        Ordering::Less => -1,
        Ordering::Equal => 0,
        Ordering::Greater => 1,
    }
}

pub fn test2(call: fn() -> Ordering) -> i32 {
    call() as i32
}

but compile to the following x86_64 assembly on play.rust-lang.org:

playground::test1:
	pushq	%rax
	callq	*%rdi
	incb	%al
	movzbl	%al, %eax
	decl	%eax
	popq	%rcx
	retq

playground::test2:
	pushq	%rax
	callq	*%rdi
	movsbl	%al, %eax
	popq	%rcx
	retq

for both stable (1.66.0) and nightly (1.68.0-nightly 2023-01-03 c757267).

[nikic]

Godbolt: https://rust.godbolt.org/z/3qqrhehTr
Missing this fold: https://alive2.llvm.org/ce/z/7p4sQg

We do handle the general pattern: https://llvm.godbolt.org/z/7MndEd3an The problem here is that the zext could also be an sext due to the range restriction.

[nikic]

Though this one could also be solved by converting the phi into sext in InstCombine already -- pretty sure we needed that to solve some more general cases of this problem.

[nikic]

Though this one could also be solved by converting the phi into sext in InstCombine already -- pretty sure we needed that to solve some more general cases of this problem.

In particular the case where we don't have range metadata can't be solved by just folding the add/zext/add pattern. We either need to recognize sext in simplifyUsingControlFlow(), or directly produce sext when SimplifyCFG removes the switch.

[nikic]

We have llvm/llvm-project#54561 for this case and llvm/llvm-project#59671 for a more complicated case with an extra * -1.

[scottmcm]

Added mir-opt because we could consider recognizing simple matches like this in MIR as well, replacing them with https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.Rvalue.html#variant.Discriminant.

That might have the advantage that we have a Rust type, and thus can tell whether to look for sext or zext of the values.

[nikic]

We do handle the general pattern: https://llvm.godbolt.org/z/7MndEd3an The problem here is that the zext could also be an sext due to the range restriction.

This should have said "sext could also be a zext". This should be very easy to fix now, because it will be a zext nneg, so the pattern to handle would be https://alive2.llvm.org/ce/z/2iCmDs. Just need to find where the SExt variant is folded and check for SExtLike instead.

[dianqk]

We do handle the general pattern: https://llvm.godbolt.org/z/7MndEd3an The problem here is that the zext could also be an sext due to the range restriction.

This should have said "sext could also be a zext". This should be very easy to fix now, because it will be a zext nneg, so the pattern to handle would be https://alive2.llvm.org/ce/z/2iCmDs. Just need to find where the SExt variant is folded and check for SExtLike instead.

Although, we need to fix that. But I found that transformation on switch is also appropriate. I can expand the original issue into the following kinds.

#![crate_type = "lib"]

#[no_mangle]
pub unsafe fn foo(a: i32) -> i8 {
    match a {
        1 => 1,
        9 => 9,
        _ => std::hint::unreachable_unchecked()
    }
}

#[no_mangle]
pub unsafe fn bar(a: i32) -> i8 {
    match a {
        -1 => -1,
        3 => 3,
        9 => 9,
        _ => std::hint::unreachable_unchecked()
    }
}

https://rust.godbolt.org/z/KTsc53bqz

The first transformation did not make better use of the UB and could not continue the transformation after it became a compare instruction. Although I can handle both scenarios in LLVM, but I can make the transformation easier in Rust by utilizing the #formation of the integers.

[dianqk]

We do handle the general pattern: https://llvm.godbolt.org/z/7MndEd3an The problem here is that the zext could also be an sext due to the range restriction.

This should have said "sext could also be a zext". This should be very easy to fix now, because it will be a zext nneg, so the pattern to handle would be https://alive2.llvm.org/ce/z/2iCmDs. Just need to find where the SExt variant is folded and check for SExtLike instead.

I created an issue for this: llvm/llvm-project#88348.

[dianqk]

@rustbot claim