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

Codegen regression involving assume/unreachable_unchecked #110551

Closed
WaffleLapkin opened this issue Apr 19, 2023 · 6 comments · Fixed by #110569
Closed

Codegen regression involving assume/unreachable_unchecked #110551

WaffleLapkin opened this issue Apr 19, 2023 · 6 comments · Fixed by #110569
Assignees
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@WaffleLapkin
Copy link
Member

Code

I tried this code:

pub fn f(a: Option<u32>) -> u32 {
    match a {
        None => {}
        Some(_) => unsafe { std::hint::unreachable_unchecked() }
    }

    a.unwrap()
}

I expected to see this happen: no branch instructions emitted, unreachable_unchecked makes an assumption that a is None, so unwrap should be inlined and optimized as a panic.

Instead, this happened: there is a discriminant check still emitted.

Version it worked on

It most recently worked on: nightly-2023-03-26

Version with regression

; rustc +nightly-2023-03-27 --version --verbose
rustc 1.70.0-nightly (db0cbc48d 2023-03-26)
binary: rustc
commit-hash: db0cbc48d4aaa300713a95d9b317a365a474490c
commit-date: 2023-03-26
host: aarch64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.0

ASM diff

See godbolt: https://godbolt.org/z/5WKTY99Te

Bisection results

Regression occurred in #106428 (cc @saethlin).

searched nightlies: from nightly-2023-03-01 to nightly-2023-04-01
regressed nightly: nightly-2023-03-27
searched commit range: 0c61c7a...db0cbc4
regressed commit: 2420bd3

bisected with cargo-bisect-rustc v0.6.6

Host triple: aarch64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2023-03-01 --end=2023-04-01 --regress=success --script=./t.sh
t.sh
#!/bin/sh

set -ex

rustc t.rs --crate-type=lib --emit=asm -O && cat ./t.s | grep -E "cbz\s+w0, .LBB0_2"

(if you want to test on x86_64 replace "cbz\s+w0, .LBB0_2" with "testl\s+%edi, %edi")

t.rs
pub fn f(a: Option<i32>) -> i32 {
    unsafe { if !a.is_none() { std::hint::unreachable_unchecked() } }

    return a.unwrap();
}

History

@heckad originally noticed the problem and opened #110456, then @WaffleLapkin suggested that this is a compiler issue, then @bugadani noticed that this is a regression, then @WaffleLapkin bisected and opened this issue.

@WaffleLapkin WaffleLapkin added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. A-mir-opt-inlining Area: MIR inlining labels Apr 19, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 19, 2023
@saethlin
Copy link
Member

searched nightlies: from nightly-2023-03-01 to nightly-2023-04-01

The end of this regression window is over 2 weeks from now. Why?

Godbolt usually has pretty old nightlies. If I add --emit=mir: https://godbolt.org/z/YfxfhETfb you can see we have duplicate switch targets, which means that either this commit is broken or godbolt doesn't have it: 2a628bd

@lqd
Copy link
Member

lqd commented Apr 19, 2023

godbolt seems to have rustc 1.71.0-nightly (d0f204e4d 2023-04-16) ?

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Apr 19, 2023

searched nightlies: from nightly-2023-03-01 to nightly-2023-04-01

The end of this regression window is over 2 weeks from now. Why?

Because I knew that 2023-04-01 reproduces the regression, and wanted to cut some time bisecting. For reference I can reproduce the same issue with +nightly-2023-04-19.

+nightly-2023-04-19 --emit=mir
// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn f(_1: Option<u32>) -> u32 {
    debug a => _1;                       // in scope 0 at t.rs:1:10: 1:11
    let mut _0: u32;                     // return place in scope 0 at t.rs:1:29: 1:32
    let mut _2: isize;                   // in scope 0 at t.rs:3:9: 3:13
    scope 1 {
        scope 2 (inlined unreachable_unchecked) { // at t.rs:4:28: 4:62
            scope 3 {
                scope 4 (inlined unreachable_unchecked::runtime) { // at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/intrinsics.rs:2495:13: 2495:80
                }
            }
        }
    }
    scope 5 (inlined #[track_caller] Option::<u32>::unwrap) { // at t.rs:7:7: 7:15
        debug self => _1;                // in scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:947:25: 947:29
        let mut _3: isize;               // in scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:949:13: 949:22
        let mut _4: !;                   // in scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:950:21: 950:73
        scope 6 {
            debug val => _0;             // in scope 6 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:949:18: 949:21
        }
    }

    bb0: {
        _2 = discriminant(_1);           // scope 0 at t.rs:2:11: 2:12
        switchInt(move _2) -> [0: bb2, 1: bb1, otherwise: bb1]; // scope 0 at t.rs:2:5: 2:12
    }

    bb1: {
        unreachable;                     // scope 3 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/intrinsics.rs:2480:9: 2496:10
    }

    bb2: {
        _3 = discriminant(_1);           // scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:948:15: 948:19
        switchInt(move _3) -> [0: bb3, 1: bb4, otherwise: bb1]; // scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:948:9: 948:19
    }

    bb3: {
        _4 = core::panicking::panic(const "called `Option::unwrap()` on a `None` value"); // scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:950:21: 950:73
                                         // mir::Constant
                                         // + span: /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:950:21: 950:26
                                         // + literal: Const { ty: fn(&'static str) -> ! {core::panicking::panic}, val: Value(<ZST>) }
                                         // mir::Constant
                                         // + span: /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:950:27: 950:72
                                         // + literal: Const { ty: &str, val: Value(Slice(..)) }
    }

    bb4: {
        _0 = ((_1 as Some).0: u32);      // scope 5 at /rustc/c609da59d9fc05b1c7dc879d79700ccd8140b5fc/library/core/src/option.rs:949:18: 949:21
        return;                          // scope 0 at t.rs:8:2: 8:2
    }
}

@saethlin
Copy link
Member

saethlin commented Apr 19, 2023

To be clear, separate from this issue I believe that this MIR:

    bb0: {
        _2 = discriminant(_1);           // scope 0 at t.rs:2:11: 2:12
        switchInt(move _2) -> [0: bb2, 1: bb1, otherwise: bb1]; // scope 0 at t.rs:2:5: 2:12
    }

is a bug.

I do not know what impact resolving that would have on this issue. But the InstCombine helper that combines switch targets was (notionally) supposed to prevent this.

@saethlin saethlin self-assigned this Apr 19, 2023
@saethlin
Copy link
Member

saethlin commented Apr 19, 2023

Yeah, I'm pretty sure that is the root cause.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 20, 2023
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 20, 2023
@bors bors closed this as completed in 4a03f14 Apr 21, 2023
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Deduplicate unreachable blocks, for real this time

In rust-lang/rust#106428 (in particular rust-lang/rust@41eda69) we noticed that inlining `unreachable_unchecked` can produce duplicate unreachable blocks. So we improved two MIR optimizations: `SimplifyCfg` was given a simplify to deduplicate unreachable blocks, then `InstCombine` was given a combiner to deduplicate switch targets that point at the same block. The problem is that change doesn't actually work.

Our current pass order is
```
SimplifyCfg (does nothing relevant to this situation)
Inline (produces multiple unreachable blocks)
InstCombine (doesn't do anything here, oops)
SimplifyCfg (produces the duplicate SwitchTargets that InstCombine is looking for)
```

So in here, I have factored out the specific function from `InstCombine` and placed it inside the simplify that produces the case it is looking for. This should ensure that it runs in the scenario it was designed for.

Fixes rust-lang/rust#110551
r? `@cjgillot`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Deduplicate unreachable blocks, for real this time

In rust-lang/rust#106428 (in particular rust-lang/rust@41eda69) we noticed that inlining `unreachable_unchecked` can produce duplicate unreachable blocks. So we improved two MIR optimizations: `SimplifyCfg` was given a simplify to deduplicate unreachable blocks, then `InstCombine` was given a combiner to deduplicate switch targets that point at the same block. The problem is that change doesn't actually work.

Our current pass order is
```
SimplifyCfg (does nothing relevant to this situation)
Inline (produces multiple unreachable blocks)
InstCombine (doesn't do anything here, oops)
SimplifyCfg (produces the duplicate SwitchTargets that InstCombine is looking for)
```

So in here, I have factored out the specific function from `InstCombine` and placed it inside the simplify that produces the case it is looking for. This should ensure that it runs in the scenario it was designed for.

Fixes rust-lang/rust#110551
r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants