Skip to content

Codegen regression involving assume/unreachable_unchecked #110551

Closed
#110569
Closed
Codegen regression involving assume/unreachable_unchecked#110551
#110569
Assignees
saethlin
Labels
A-mir-opt-inliningArea: MIR inliningC-bugCategory: This is a bug.P-mediumMedium priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.

Description

@WaffleLapkin
WaffleLapkin
opened on Apr 19, 2023

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.

Activity

  1. WaffleLapkin
    added
    regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.
    C-bugCategory: This is a bug.
    A-mir-opt-inliningArea: MIR inlining
    on Apr 19, 2023
  2. rustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Apr 19, 2023

  3. saethlin commented on Apr 19, 2023

    @saethlin
    saethlin
    on Apr 19, 2023
    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

  4. lqd commented on Apr 19, 2023

    @lqd
    lqd
    on Apr 19, 2023
    Member

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

  5. WaffleLapkin commented on Apr 19, 2023

    @WaffleLapkin
    WaffleLapkin
    on Apr 19, 2023
    MemberAuthor

    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
    }
}

  6. saethlin commented on Apr 19, 2023

    @saethlin
    saethlin
    on Apr 19, 2023
    Member

    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.

  7. saethlin
    self-assigned this
    on Apr 19, 2023

  8. saethlin commented on Apr 19, 2023

    @saethlin
    saethlin
    on Apr 19, 2023
    Member

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

  9. apiraino commented on Apr 20, 2023

    @apiraino
    apiraino
    on Apr 20, 2023
    Contributor

    WG-prioritization assigning priority (Zulip discussion).

    @rustbot label -I-prioritize +P-medium

  10. rustbot
    added
    P-mediumMedium priority
    and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Apr 20, 2023
  11. apiraino
    added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Apr 20, 2023
  12. bors
    closed this as completedin 4a03f14on Apr 21, 2023
  13. RalfJung
    added a commit that references this issue on Apr 20, 2024

    Auto merge of #110569 - saethlin:mir-pass-cooperation, r=cjgillot

    d8f4d1a
  14. RalfJung
    added a commit that references this issue on Apr 27, 2024

    Auto merge of #110569 - saethlin:mir-pass-cooperation, r=cjgillot

    8a76857
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-mir-opt-inliningArea: MIR inliningC-bugCategory: This is a bug.P-mediumMedium priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions