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 when removing intermediate #116150

Open
ojeda opened this issue Sep 25, 2023 · 2 comments
Open

Missed optimization when removing intermediate #116150

ojeda opened this issue Sep 25, 2023 · 2 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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

@ojeda
Copy link
Contributor

ojeda commented Sep 25, 2023

Using Rust 1.72.0:

pub fn f(err: i32) -> Result<u16, i32> {
    if err < 0 {
        return Err(err);
    }
    
    Ok(err as u16)
}

compiles to:

example::f:
        mov     ecx, edi
        shl     ecx, 16
        xor     eax, eax
        test    edi, edi
        cmovns  eax, ecx
        mov     ecx, edi
        shr     ecx, 31
        shl     rdi, 32
        or      rax, rdi
        or      rax, rcx
        ret

But adding an intermediate before the conditional appears to help:

pub fn f(err: i32) -> Result<u16, i32> {
    let ok = err as u16;

    if err < 0 {
        return Err(err);
    };
    
    Ok(ok)
}

compiles to:

example::f:
        mov     ecx, edi
        shr     ecx, 31
        mov     rax, rdi
        shl     rax, 32
        shl     edi, 16
        or      rax, rdi
        or      rax, rcx
        ret

https://godbolt.org/z/bf6jW9d3c

This intermediate could be an argument in a call, which is in fact how I originally noticed it, i.e. this:

fn to_result_ok<T>(err: i32, ok: T) -> Result<T, i32> {
    if err < 0 {
        return Err(err);
    }
    
    Ok(ok)
}

pub fn f(ret: i32) -> Result<u16, i32> {
    to_result_ok(ret, ret as u16)
}

compiles better than:

pub fn f(ret: i32) -> Result<u16, i32> {
    if ret < 0 {
        return Err(ret);
    }

    Ok(ret as u16)
}
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 25, 2023
@bjorn3
Copy link
Member

bjorn3 commented Sep 25, 2023

In the second case the conditional is impossible as an unsigned int is never less than 0.

@ojeda
Copy link
Contributor Author

ojeda commented Sep 25, 2023

The condition is not on ok, but on err. That is why it is quite surprising! :)

@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 25, 2023
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. 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

4 participants