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

Discriminant update fails to optimize #122600

Closed
joboet opened this issue Mar 16, 2024 · 7 comments · Fixed by #128500
Closed

Discriminant update fails to optimize #122600

joboet opened this issue Mar 16, 2024 · 7 comments · Fixed by #128500
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@joboet
Copy link
Member

joboet commented Mar 16, 2024

The following code should optimize to a single store of the new discriminant, but doesn't (Godbolt).

enum State {
    A([u8; 753]),
    B([u8; 753]),
    //C,
}

unsafe fn update(s: *mut State) {
    let S::A(v) = s.read() else { unreachable_unchecked() };
    s.write(S::B(v));
}

Instead, LLVM copies the value to the stack, copies it again and then writes it back. Interestingly, LLVM optimizes this correctly if a third state without value is added.

Meta

rustc --version --verbose:

rustc 1.78.0-nightly (c67326b06 2024-03-15)
binary: rustc
commit-hash: c67326b063bd27ed04f306ba2e372cd92e0a8751
commit-date: 2024-03-15
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0
@joboet joboet added I-slow Issue: Problems and improvements with respect to performance of generated code. C-bug Category: This is a bug. labels Mar 16, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 16, 2024
@matthiaskrgr
Copy link
Member

lol, looks like this optimizes with -Zmir-opt=1/0 but fails to with anything equals or above 2, I think 2 or 3 is the default for release builds?

@matthiaskrgr matthiaskrgr added the A-mir-opt Area: MIR optimizations label Mar 16, 2024
@clubby789
Copy link
Contributor

clubby789 commented Mar 16, 2024

Replacing unreachable_unchecked with return also optimizes better:

init:
        test    byte ptr [rdi], 1
        jne     .LBB0_2
        mov     byte ptr [rdi], 1
.LBB0_2:
        ret

Presumably some issue with MIR inlining;

#[inline(never)]
fn unreachable2() -> ! {
    unsafe { unreachable_unchecked() }
}

#[no_mangle]
unsafe fn init(s: *mut State) {
    let State::A(v) = read(s) else { unreachable2() };
    write(s, State::B(v));
}

produces the optimal code

@clubby789 clubby789 added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 16, 2024
@saethlin
Copy link
Member

saethlin commented Mar 16, 2024

The problem is not MIR inlining per se. I think this is the problem @DianQK was trying to fix in #122282; if you write your own unreachable_unchecked function without a runtime check, it optimizes fine: https://godbolt.org/z/6oYqrhEs9

@saethlin saethlin self-assigned this Mar 16, 2024
@saethlin saethlin linked a pull request Mar 16, 2024 that will close this issue
@saethlin saethlin removed their assignment Apr 29, 2024
@DianQK
Copy link
Member

DianQK commented Jul 3, 2024

Upstream issue: llvm/llvm-project#85560

@rustbot claim
:p

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

Error: Parsing assign command in comment failed: ...' claim' | error: expected end of command at >| ' :p'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@DianQK
Copy link
Member

DianQK commented Jul 13, 2024

@rustbot label +llvm-fixed-upstream

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Jul 13, 2024
@nikic
Copy link
Contributor

nikic commented Aug 1, 2024

Confirmed fixed by #127513, needs codegen test.

@nikic nikic added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. and removed llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes labels Aug 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 4, 2024
…lacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 4, 2024
…lacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 4, 2024
…lacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 4, 2024
…lacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 5, 2024
…lacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
@bors bors closed this as completed in 3c8b259 Aug 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2024
Rollup merge of rust-lang#128500 - clubby789:122600-test, r=Mark-Simulacrum

Add test for updating enum discriminant through pointer

Closes rust-lang#122600
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
7 participants