rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.54k stars 12.61k forks source link

Adding an unreachable branch helps optimize the code when matching on `x % N` #93514

Open WaffleLapkin opened 2 years ago

WaffleLapkin commented 2 years ago

Consider this example:

pub fn parse(x: usize) -> usize {
    match x % 5 {
        0 => f1(x),
        1 => f2(x),
        2 => f3(x),
        3 => f4(x),
        4 => f5(x),
        // 5 | 6 | 7 => loop{},
        _ => loop{},
    }
}

It currently generates similar LLVM-IR:

start:
  %_2 = urem i64 %x, 5, !dbg !10
  switch i64 %_2, label %bb12 [
    i64 0, label %bb2
    i64 1, label %bb4
    i64 2, label %bb6
    i64 3, label %bb8
    i64 4, label %bb10
  ], !dbg !11
; ...
bb12:
  br label %bb12, !dbg !32

Even though the default branch is unreachable (x % 5 can't be greater than 4) it is still generated.

But, when un-commenting 5 | 6 | 7 => loop{} line (still unreachable branch, that does the same as default) the default branch becomes unreachable:

start:
  %_2 = urem i64 %x, 5, !dbg !10
  switch i64 %_2, label %bb14 [
    i64 0, label %bb2
    i64 1, label %bb4
    i64 2, label %bb6
    i64 3, label %bb8
    i64 4, label %bb10
  ], !dbg !11
; ...
bb14:
  unreachable

So, adding an unreachable branch that does the same as the default helps optimize the code.

Some notes:

WaffleLapkin commented 2 years ago

@rustbot label +A-codegen +C-enhancement +I-slow +T-compiler

scottmcm commented 2 years ago

I think there's an LLVM bug here, despite your clang example.

Here's a demonstration using just opt that it doesn't notice the defaultdest is unreachable, and thus leaves the infinite loop there: https://llvm.godbolt.org/z/GcG1dM3sq

erikdesjardins commented 2 years ago

In LLVM, when the optimization fires, the unreachable bb is replaced with .unreachabledefault (https://llvm.godbolt.org/z/jWeeWYKhE), so we can tell it's created by createUnreachableSwitchDefault, called from here: https://github.com/llvm/llvm-project/blob/c25ba3c79020246b24aa658ac02770be07eee0f5/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L4990.

That code determines whether or not the default condition is reachable based on the number of significant bits in the condition, which is why it doesn't work until you have power-of-2-minus-1 cases. It could be made more precise. (I'm looking into this.)

nikic commented 2 years ago

I believe https://reviews.llvm.org/D106056 has already fixed this. Unfortunately, this exposed some compile-time issues in the backend, which haven't been resolved yet, so the patch is reverted for now.

erikdesjardins commented 8 months ago

Looks like this will be handled better in LLVM 18 (after #120055): https://godbolt.org/z/reaf5oKcb

nikic commented 8 months ago

@erikdesjardins I believe the relevant change has been reverted.

mati865 commented 8 months ago

There is a PR that adds unreachable like that: https://github.com/rust-lang/rust/pull/120268

DianQK commented 8 months ago

There is a PR that adds unreachable like that: #120268

It only solves enum scenarios. https://github.com/llvm/llvm-project/pull/76669 will partially solve the issue. It was also reverted due to some codegen issues.

DianQK commented 8 months ago

I believe https://reviews.llvm.org/D106056 has already fixed this. Unfortunately, this exposed some compile-time issues in the backend, which haven't been resolved yet, so the patch is reverted for now.

Looks like the same issue I'm having.

DianQK commented 8 months ago

There is a PR that adds unreachable like that: #120268

It only solves enum scenarios. The enum scenario has been solved, this PR is an extension.

https://github.com/llvm/llvm-project/pull/76669 will partially solve the issue. It was also reverted due to some backend issues.

erikdesjardins commented 2 months ago

llvm/llvm-project#76669 will partially solve the issue. It was also reverted due to some backend issues.

That change got unreverted and was included in the recent LLVM 19 upgrade, so I believe this is now fixed: https://godbolt.org/z/1MvffeWff

@rustbot label E-needs-test

DianQK commented 2 months ago

This is probably fixed by https://github.com/llvm/llvm-project/pull/79993.

Hmm, looks like 1.78 has fixed this: https://rust.godbolt.org/z/n4fbG4xj4.