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

Missed optimization: Option<fieldless enum> equality #72646

Closed
Vlad-Shcherbina opened this issue May 27, 2020 · 3 comments
Closed

Missed optimization: Option<fieldless enum> equality #72646

Vlad-Shcherbina opened this issue May 27, 2020 · 3 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Vlad-Shcherbina
Copy link

This is similar to #49892, but different.
It's not limited to non-zero integer types. This happens for any types that have holes but no uninitialized bits.
I think comparing Option<some fieldless enum> values is not uncommon, so it adds to the severity of the issue.

To reproduce

  1. Create the following program:
    pub fn eq(x: Option<bool>, y: Option<bool>) -> bool {
        x == y
    }
  2. Inspect generated code, for example by running
    cargo rustc --release --lib -- --emit asm -Cllvm-args=--x86-asm-syntax=intel
    
    Or just visit godbolt.

Expected result

eq:
        cmp     dil, sil
        sete    al
        ret

Actual result

eq:
        cmp     dil, 2
        sete    al
        cmp     sil, 2
        setne   cl
        cmp     al, cl
        je      .LBB0_1
        mov     al, 1
        cmp     dil, 2
        je      .LBB0_5
        cmp     sil, 2
        je      .LBB0_5
        test    dil, dil
        sete    cl
        test    sil, sil
        setne   al
        xor     al, cl
.LBB0_5:
        ret
.LBB0_1:
        xor     eax, eax
        ret

Meta

This happens both on 1.43.0 stable and on the recent nightly:

> rustc --version --verbose
rustc 1.45.0-nightly (8970e8bcf 2020-05-23)
binary: rustc
commit-hash: 8970e8bcf6153d1ead2283f1a0ed7b192230eca6
commit-date: 2020-05-23
host: x86_64-pc-windows-msvc
release: 1.45.0-nightly
LLVM version: 10.0
@Vlad-Shcherbina Vlad-Shcherbina added the C-bug Category: This is a bug. label May 27, 2020
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. labels May 27, 2020
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
@veera-sivarajan
Copy link
Contributor

veera-sivarajan commented Feb 25, 2025

This is same as #122734

https://rust.godbolt.org/z/MbvvK8e59

@rustbot label +a-llvm

@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Feb 25, 2025
veera-sivarajan added a commit to llvm/llvm-project that referenced this issue Mar 12, 2025
Proof: https://alive2.llvm.org/ce/z/U-G7yV

Helps: rust-lang/rust#72646 and
rust-lang/rust#122734

  Rust compiler's current output: https://godbolt.org/z/7E3fET6Md

IPSCCP can do this transform but it does not help the motivating issue
since it runs only once early in the optimization pipeline.

Reimplementing this in CVP folds the motivating issue into a simple
`icmp eq` instruction.
  
  Fixes #130100
@veera-sivarajan
Copy link
Contributor

@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 Mar 12, 2025
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue Mar 12, 2025
Proof: https://alive2.llvm.org/ce/z/U-G7yV

Helps: rust-lang/rust#72646 and
rust-lang/rust#122734

  Rust compiler's current output: https://godbolt.org/z/7E3fET6Md

IPSCCP can do this transform but it does not help the motivating issue
since it runs only once early in the optimization pipeline.

Reimplementing this in CVP folds the motivating issue into a simple
`icmp eq` instruction.

  Fixes #130100
frederik-h pushed a commit to frederik-h/llvm-project that referenced this issue Mar 18, 2025
Proof: https://alive2.llvm.org/ce/z/U-G7yV

Helps: rust-lang/rust#72646 and
rust-lang/rust#122734

  Rust compiler's current output: https://godbolt.org/z/7E3fET6Md

IPSCCP can do this transform but it does not help the motivating issue
since it runs only once early in the optimization pipeline.

Reimplementing this in CVP folds the motivating issue into a simple
`icmp eq` instruction.
  
  Fixes llvm#130100
@scottmcm
Copy link
Member

I know this bug is the original, but it looks like #122734 got more discussion.

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. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants