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

Rust generates suboptimal code for x.div_euclid(2) #118392

Closed
KamilaBorowska opened this issue Nov 27, 2023 · 6 comments · Fixed by #125347
Closed

Rust generates suboptimal code for x.div_euclid(2) #118392

KamilaBorowska opened this issue Nov 27, 2023 · 6 comments · Fixed by #125347
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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-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

@KamilaBorowska
Copy link
Contributor

I tried this code:

pub fn div2(a: i32) -> i32 {
    a.div_euclid(2)
}

I expected to see this happen:

After compiling to assembly with -C opt-level=3 to generate the following x86 assembly code.

div2:
        mov     eax, edi
        sar     eax
        ret

Instead, this happened:

div2:
        mov     eax, edi
        shr     eax, 31
        add     eax, edi
        sar     eax
        not     edi
        and     edi, -2147483647
        cmp     edi, 1
        sbb     eax, 0
        ret

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
@KamilaBorowska KamilaBorowska added the C-bug Category: This is a bug. label Nov 27, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 27, 2023
@Urgau
Copy link
Member

Urgau commented Nov 27, 2023

Those are not equivalent! (EDIT: was wrong, see below) div_euclid say it it will round towards negative infinity if self < 0; while self / 2 always rounds towards positive infinity.

A simple playground example with -7 proves that they do not return the same value:

fn main() {
    assert_eq!(
        (-7i32) / 2, // -3 
        (-7i32).div_euclid(2) // -4
    );
}

However if you use u32 (or any unsigned integer) you will get the expected assembly.

@rustbot labels -C-bug -needs-triage +C-discussion

@rustbot rustbot added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 27, 2023
@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Nov 27, 2023

Those are not equivalent!

@Urgau I believe x.div_euclid(2) and x >> 1 (not to be confused with x / 2) are in fact equivalent, and Alive2 tells me that this optimization is valid (https://alive2.llvm.org/ce/z/YVeJvZ), albeit I tested it with i8 (testing with i32 gets a timeout).

x / 2 on the other hand compiles down to the following:

div2:
        mov     eax, edi
        shr     eax, 31
        add     eax, edi
        sar     eax
        ret

Note how the generated code adds sign bit, which is correct. x / 2 is not the same thing as x >> 1 specifically because it rounds towards 0, and not negative infinity.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Nov 28, 2023

@rustbot labels +C-bug +needs-triage -C-discussion I-heavy I-slow A-LLVM A-codegen T-compiler

@rustbot rustbot added A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. 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. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed C-discussion Category: Discussion or questions that doesn't represent real issues. labels Nov 28, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 28, 2023
@nikic
Copy link
Contributor

nikic commented Nov 28, 2023

LLVM issue: llvm/llvm-project#73622 Worth noting that the problem only occurs with 2, not other power of two constants.

@Urgau
Copy link
Member

Urgau commented Nov 28, 2023

@Urgau I believe x.div_euclid(2) and x >> 1 (not to be confused with x / 2) are in fact equivalent, and Alive2 tells me that this optimization is valid (https://alive2.llvm.org/ce/z/YVeJvZ), albeit I tested it with i8 (testing with i32 gets a timeout).

My bad, I wrongly deduced that you meant x / 2 not x >> 1. I even checked under Alive2. 👀

@nikic nikic added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Nov 30, 2023
@nikic
Copy link
Contributor

nikic commented Feb 14, 2024

Confirmed fixed by #120055, 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 Feb 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 10, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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-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

Successfully merging a pull request may close this issue.

6 participants