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

Pattern match incorrectly optimized out (bad range info handling in LLVM?) #26468

Closed
rprichard opened this issue Jun 21, 2015 · 10 comments · Fixed by #27140
Closed

Pattern match incorrectly optimized out (bad range info handling in LLVM?) #26468

rprichard opened this issue Jun 21, 2015 · 10 comments · Fixed by #27140

Comments

@rprichard
Copy link
Contributor

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
#[allow(dead_code)]
enum BarMode {
    Bar1,
    Bar2,
}

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
#[allow(dead_code)]
enum Mode {
    Foo(u8),
    Bar(BarMode),
}

#[inline(never)]
fn broken(mode: &Mode) -> u32 {
    for _ in 0..1 {
        match *mode {
            Mode::Bar(BarMode::Bar1) => { return 17 }
            Mode::Foo(5) => { return 19 }
            _ => {}
        }
    }
    return 42;
}

fn main() {
    let mode = Mode::Foo(5);
    assert_eq!(broken(&mode), 19);
}

This test program should exit quietly, but with rustc -O, it instead fails the assertion:

thread '<main>' panicked at 'assertion failed: `(left == right) && (right == left)` (left: `42`, right: `19`)', redux.rs:29
rprichard@ryan:~/mess/redux$ rustc --version --verbose
rustc 1.2.0-nightly (20d23d8e5 2015-06-18)
binary: rustc
commit-hash: 20d23d8e57c0313c811135535d6872463cc6525d
commit-date: 2015-06-18
host: x86_64-unknown-linux-gnu
release: 1.2.0-nightly

I also tested a really old 32-bit compiler, and it also failed:

rprichard@ryan:~/mess/redux$ rustn32 --version --verbose
rustc 1.0.0-nightly (c89de2c56 2015-03-28) (built 2015-03-29)
binary: rustc
commit-hash: c89de2c56baeb61e7cc434924dcc8bedd32b26b8
commit-date: 2015-03-28
build-date: 2015-03-29
host: i686-unknown-linux-gnu
release: 1.0.0-nightly

This test case is reduced from #25919.

@Stebalien
Copy link
Contributor

Probably a dup of #26251 (and #18060)

@rprichard
Copy link
Contributor Author

Perhaps it's a dup. The other two issues are broken with and without optimizations, though. This issue only fails with optimizations.

@rprichard
Copy link
Contributor Author

Also: the match expression in this issue doesn't actually require any overlap of the match patterns. (I'll update the test case.)

@rprichard
Copy link
Contributor Author

The bug also happens without a match expression:

#[inline(never)]
fn broken(mode: &Mode) -> u32 {
    for _ in 0..1 {
        if let Mode::Bar(BarMode::Bar1) = *mode { return 17 }
        if let Mode::Foo(5) = *mode { return 19 }
    }
    return 42;
}

@Stebalien
Copy link
Contributor

Good point. Also, this only happens with opt-level=2 (not 0 or 3).

@rprichard
Copy link
Contributor Author

cc @dotdash

Further refinement of the test case:

#[allow(dead_code)]
#[repr(u16)]
enum FooMode {
    First = 0x1000,
    Check = 0x1001,
    Last  = 0x100f,
}

#[allow(dead_code)]
#[repr(u16)]
enum BarMode {
    First = 0x2000,
    Check = 0x2001,
    Last  = 0x200f,
}

#[allow(dead_code)]
enum Mode {
    Foo(FooMode),
    Bar(BarMode),
}

#[inline(never)]
fn broken(mode: &Mode) -> u32 {
    for _ in 0..1 {
        if let Mode::Foo(FooMode::Check) = *mode { return 17 }
        if let Mode::Bar(BarMode::Check) = *mode { return 19 }
    }
    return 42;
}

fn main() {
    let mode = Mode::Bar(BarMode::Check);
    assert_eq!(broken(&mode), 19);
}

Changing FooMode::Last from 0x100f to a large enough value avoids the bug. Apparently, the value needs to be large enough that, when rounded to the next highest power-of-two-minus-one, it is at least as large as BarMode::Check.

I think this is an LLVM bug. It involves the !range metadata. It's as if the compiler is reusing the range information from the Mode::Foo(FooMode::Check) check to elide the Mode::Bar(BarMode::Check) check (which is missing from the x64 assembly). Relevant unoptimized LLVM IR:

match_case7:                                      ; preds = %case_body2
  %26 = bitcast %Mode* %21 to { i16, i16 }*
  %27 = getelementptr inbounds { i16, i16 }, { i16, i16 }* %26, i32 0, i32 1
  %28 = load i16, i16* %27, !range !3
  switch i16 %28, label %match_else8 [
    i16 4097, label %match_case9
  ]

...

match_case14:                                     ; preds = %join
  %32 = bitcast %Mode* %29 to { i16, i16 }*
  %33 = getelementptr inbounds { i16, i16 }, { i16, i16 }* %32, i32 0, i32 1
  %34 = load i16, i16* %33, !range !4
  switch i16 %34, label %match_else15 [
    i16 8193, label %match_case16
  ]

...

!3 = !{i16 4096, i16 4112}
!4 = !{i16 8192, i16 8208}

FooMode::Last isn't used in the test program. When I change its value, the only difference I notice in the unoptimized LLVM IR is in the range metadata (i.e. !3).

@rprichard rprichard changed the title Wrong code generated with optimizations Pattern match incorrectly optimized out (bad range info handling in LLVM?) Jun 21, 2015
@dotdash
Copy link
Contributor

dotdash commented Jun 22, 2015

Thanks Ryan! Indeed LLVM replaces the load with another disregarding the range information.

@dotdash
Copy link
Contributor

dotdash commented Jun 22, 2015

@dotdash
Copy link
Contributor

dotdash commented Jul 10, 2015

Patch landed upstream.

dotdash added a commit to dotdash/rust that referenced this issue Jul 20, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 21, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 21, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 21, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 22, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 22, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 22, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
steveklabnik added a commit to steveklabnik/rust that referenced this issue Jul 22, 2015
The fix for rust-lang#26468 was made upstream and landed with the LLVM update in rust-lang#27076.

Closes rust-lang#26468
@MagaTailor
Copy link

Using yesterday's nightly, make test fails on armv7 at this single test. The problem seems to be local llvm 3.6 is still allowed by the build system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants