Skip to content

JumpThreading is confused by an extraordinary use of SetDiscriminant #119674

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
tmiasko opened this issue Jan 6, 2024 · 2 comments · Fixed by #119675
Closed

JumpThreading is confused by an extraordinary use of SetDiscriminant #119674

tmiasko opened this issue Jan 6, 2024 · 2 comments · Fixed by #119675
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Jan 6, 2024

#![feature(custom_mir)]
#![feature(core_intrinsics)]
use std::intrinsics::mir::*;

enum E { A, B(char) }

#[custom_mir(dialect = "runtime")]
pub fn f() -> usize {
    mir!(
        let a: isize;
        let e: E;
        {
            e = E::A;
            SetDiscriminant(e, 1);
            a = Discriminant(e);
            match a {
                0 => bb0,
                _ => bb1,
            }
        }
        bb0 = {
            RET = 0;
            Return()
        }
        bb1 = {
            RET = 1;
            Return()
        }
    )
}

fn main() {
    assert_eq!(f(), 0);
}
$ rustc c.rs -Zmir-opt-level=0 && ./c
$ rustc c.rs -Zmir-opt-level=0 -Zmir-enable-passes=+JumpThreading && ./c
thread 'main' panicked at c.rs:33:5:
assertion `left == right` failed
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @cjgillot

Meta

rustc --version --verbose:

rustc 1.77.0-nightly (595bc6f00 2024-01-05)
binary: rustc
commit-hash: 595bc6f00369475047538fdae1ff8cea692ac385
commit-date: 2024-01-05
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6
@tmiasko tmiasko added C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations labels Jan 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 6, 2024
@cjgillot
Copy link
Contributor

cjgillot commented Jan 6, 2024

TIL that SetDiscriminant does not always set the discriminant, depending on the exact layout. It's a pity that we do not have a round-trip here. This implies that we can never thread on SetDiscriminant.

I'm preparing a PR to fix the opt.

But I really think we should amend the semantics to guarantee in polymorphic context that discriminant(x) yield d after a call to SetDiscriminant(x, d).

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 6, 2024

We can restrict SetDiscriminant to coroutines (they don't suffer from this issue since they don't use niche encoding).

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 6, 2024
@bors bors closed this as completed in 75c68cf Jan 7, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. 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.

4 participants