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 for NonZeroU32 to NonZeroUsize Conversions #119422

Closed
zmtaub opened this issue Dec 29, 2023 · 6 comments
Closed

Missed Optimization for NonZeroU32 to NonZeroUsize Conversions #119422

zmtaub opened this issue Dec 29, 2023 · 6 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-heavy Issue: Problems and improvements with respect to binary size of generated code. 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.

Comments

@zmtaub
Copy link

zmtaub commented Dec 29, 2023

I tried this code:

use std::num::*;

pub fn f0(x: u32) -> u64 {
    x.try_into().unwrap()
}

pub fn f1(x: NonZeroU32) -> NonZeroU64 {
    x.try_into().unwrap()
}

pub fn f2(x: u32) -> usize {
    x.try_into().unwrap()
}

pub fn f3(x: NonZeroU32) -> NonZeroUsize {
    x.try_into().unwrap()
}

I expected to see this happen:

I expected equivalent assembly generated for each function on 64-bit platforms when compiling with -C opt-level=3.
Indeed, if f3 is removed, this assembly is generated according to Compiler Explorer:

example::f0:
        mov     eax, edi
        ret

Instead, this happened:

According to Compiler Explorer, this assembly is generated:

core::ptr::drop_in_place<core::num::error::TryFromIntError>:
        ret

example::f0:
        mov     eax, edi
        ret

example::f3:
        test    edi, edi
        je      .LBB2_1
        mov     eax, edi
        ret
.LBB2_1:
        push    rax
        lea     rdi, [rip + .L__unnamed_1]
        lea     rcx, [rip + .L__unnamed_2]
        lea     r8, [rip + .L__unnamed_3]
        lea     rdx, [rsp + 7]
        mov     esi, 43
        call    qword ptr [rip + core::result::unwrap_failed@GOTPCREL]
        ud2

.L__unnamed_1:
        .ascii  "called `Result::unwrap()` on an `Err` value"

.L__unnamed_2:
        .quad   core::ptr::drop_in_place<core::num::error::TryFromIntError>
        .asciz  "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
        .quad   <core::num::error::TryFromIntError as core::fmt::Debug>::fmt

.L__unnamed_4:
        .ascii  "/app/example.rs"

.L__unnamed_3:
        .quad   .L__unnamed_4
        .asciz  "\017\000\000\000\000\000\000\000\020\000\000\000\022\000\000"

The try_into().unwrap() isn't optimized away for f3, unlike the other functions.

Compiler Explorer link

Meta

rustc --version --verbose:

rustc 1.74.0 (79e9716c9 2023-11-13)
binary: rustc
commit-hash: 79e9716c980570bfd1f666e3b16ac583f0168962
commit-date: 2023-11-13
host: x86_64-unknown-linux-gnu
release: 1.74.0
LLVM version: 17.0.4
@zmtaub zmtaub added the C-bug Category: This is a bug. label Dec 29, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 29, 2023
@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Dec 29, 2023

@rustbot label C-optimization I-heavy I-slow T-compiler

@rustbot rustbot added C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-heavy Issue: Problems and improvements with respect to binary size of generated code. 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. labels Dec 29, 2023
@saethlin saethlin 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 Dec 29, 2023
@AngelicosPhosphoros
Copy link
Contributor

As I understand, the root cause of this is that LLVM doesn't support !range metadata on values (including function arguments) and return values.

This !range metadata is used when loading values from pointers so if we pass argument by reference, compiler optimizes code correctly: godbolt.
However, it probably would not work in practice because LLVM optimizes functions by promoting references to values in argpromotion pass.

The only good thing is that if your values are stored in memory (e.g. in some vector), it would be optimized correctly.

@AngelicosPhosphoros
Copy link
Contributor

AngelicosPhosphoros commented Dec 30, 2023

Another example: godbolt

pub fn non_zero_cmp(a: NonZeroU32)->bool {
   a.get() == 0
}

It should generate just ret i1 0 but it generates test:

example::non_zero_cmp:
        test    edi, edi
        sete    al
        ret

@zmtaub May you rename your issue to "Compiler fails to optimize NonZero integers passed as function parameters"?

@AngelicosPhosphoros
Copy link
Contributor

Made a LLVM issue.
llvm/llvm-project#76628

AngelicosPhosphoros added a commit to AngelicosPhosphoros/rust that referenced this issue Dec 30, 2023
LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
@AngelicosPhosphoros
Copy link
Contributor

I started to work on issue.
#119452

@nikic
Copy link
Contributor

nikic commented Dec 30, 2023

Duplicate of #49572.

@nikic nikic closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2023
AngelicosPhosphoros added a commit to AngelicosPhosphoros/rust that referenced this issue Dec 30, 2023
LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 30, 2023
…get_assume_nonzero, r=<try>

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
AngelicosPhosphoros added a commit to AngelicosPhosphoros/rust that referenced this issue Jan 2, 2024
LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
AngelicosPhosphoros added a commit to AngelicosPhosphoros/rust that referenced this issue Jan 6, 2024
LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 12, 2024
…get_assume_nonzero, r=<try>

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 12, 2024
…get_assume_nonzero, r=scottmcm

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang#119422
Related to llvm/llvm-project#76628
Related to rust-lang#49572
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jan 13, 2024
…e_nonzero, r=scottmcm

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang/rust#119422
Related to llvm/llvm-project#76628
Related to rust-lang/rust#49572
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…e_nonzero, r=scottmcm

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang/rust#119422
Related to llvm/llvm-project#76628
Related to rust-lang/rust#49572
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…e_nonzero, r=scottmcm

Add assume into `NonZeroIntX::get`

LLVM currently don't support range metadata for function arguments so it fails to optimize non zero integers using their invariant if they are provided using by-value function arguments.

Related to rust-lang/rust#119422
Related to llvm/llvm-project#76628
Related to rust-lang/rust#49572
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-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-heavy Issue: Problems and improvements with respect to binary size of generated code. 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.
Projects
None yet
Development

No branches or pull requests

6 participants