Bad codegen with simple match statement

[emilio]

The following code:

type CSSFloat = f32;

pub enum ViewportPercentageLength {
    Vw(CSSFloat),
    Vh(CSSFloat),
    Vmin(CSSFloat),
    Vmax(CSSFloat),
}

impl ViewportPercentageLength {
    fn try_sum(&self, other: &Self) -> Result<Self, ()> {
        use self::ViewportPercentageLength::*;
        Ok(match (self, other) {
            (&Vw(one), &Vw(other)) => Vw(one + other),
            (&Vh(one), &Vh(other)) => Vh(one + other),
            (&Vmin(one), &Vmin(other)) => Vmin(one + other),
            (&Vmax(one), &Vmax(other)) => Vmax(one + other),
            _ => return Err(()),
        })
    }
}

#[no_mangle]
pub extern "C" fn sum_them(
    one: &ViewportPercentageLength,
    other: &ViewportPercentageLength,
    out: &mut ViewportPercentageLength,
) -> bool {
    match one.try_sum(other) {
        Ok(v) => {
            *out = v;
            true
        }
        Err(()) => false,
    }
}

Generates the following assembly on Rust Nightly when compiled with -C opt-level=3:

sum_them:
        mov     eax, dword ptr [rdi]
        movss   xmm0, dword ptr [rdi + 4]
        mov     ecx, dword ptr [rsi]
        movss   xmm1, dword ptr [rsi + 4]
        lea     rsi, [rip + .LJTI0_0]
        movsxd  rax, dword ptr [rsi + 4*rax]
        add     rax, rsi
        jmp     rax
.LBB0_1:
        xor     eax, eax
        test    ecx, ecx
        je      .LBB0_8
        ret
.LBB0_3:
        mov     eax, 2
        cmp     ecx, 2
        je      .LBB0_8
.LBB0_9:
        xor     eax, eax
        ret
.LBB0_5:
        mov     eax, 3
        cmp     ecx, 3
        jne     .LBB0_9
.LBB0_8:
        addss   xmm0, xmm1
        mov     dword ptr [rdx], eax
        movss   dword ptr [rdx + 4], xmm0
        mov     al, 1
        ret
.LBB0_7:
        mov     eax, 1
        cmp     ecx, 1
        jne     .LBB0_9
        jmp     .LBB0_8
.LJTI0_0:
        .long   .LBB0_1-.LJTI0_0
        .long   .LBB0_7-.LJTI0_0
        .long   .LBB0_3-.LJTI0_0
        .long   .LBB0_5-.LJTI0_0

Godbolt link: https://rust.godbolt.org/z/JfkEez

It seems to generate one branch for each case of the statement, when I would've expected it to look more like:

if one.enum_discriminant != other.enum_discriminant {
    jump to error case
}
write enum into outparam with tag = one.error_discriminant and value one.value != other.value

cc @michaelwoerister @heycam

[emilio]

Also cc @nox / @SimonSapin as servo uses this kind of pattern quite a lot too

[emilio]

I wonder if this affects the built-in #[derive(PartialEq)] and so on.

[emilio]

Seems like PartialEq is not affected and manages to do this (I used i32 instead of f32 to avoid floating point instructions): https://rust.godbolt.org/z/DXIbja

[emilio]

However a manually implemented version of that is: https://rust.godbolt.org/z/fmYvsb

It'd be good to know what kind of code does rustc generate for this case... Does it exploit internals to poke at the representation directly?

[matthewjasper]

The code that the derive expands to is: https://rust.godbolt.org/z/P3ptVh

[pcwalton]

Might be a good candidate for a MIR opt. I believe LLVM has historically shied away from doing this kind of thing out of compile time concerns.

[emilio]

The code that the derive expands to is: https://rust.godbolt.org/z/P3ptVh

Cool, TIL!

So the unreachable is key there, and same for putting the discriminant_value check outside the match statement... Is there a way to get the discriminant value in stable without forcing #[repr(u8)] or such to be on the enum?

[SimonSapin]

std::mem::discriminant is stable and returns something that implements Eq

[emilio]

Ah, true! And std::hint::unreachable_unchecked as well... That'd work for me, I think (though improvements in the compiler itself to detect this would be awesome).

[nox]

I've always wanted to write such a peephole optimisation for match expressions over (T, T) where all the arms but the last one are (T::Foo(..), T::Foo(..)) but I didn't have the time to do it yet.

That's a low-hanging fruit and I suspect will bring improvements all across the board but days are still only 24 hours long.

[nox]

Cf this, too: servo/servo@456c18f