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

Two subsequent labels and unnecessary jmp with opt-level=s #126585

Closed
mickvangelderen opened this issue Jun 17, 2024 · 6 comments · Fixed by #128584
Closed

Two subsequent labels and unnecessary jmp with opt-level=s #126585

mickvangelderen opened this issue Jun 17, 2024 · 6 comments · Fixed by #128584
Assignees
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mickvangelderen
Copy link
Contributor

mickvangelderen commented Jun 17, 2024

The following code:

const fn checked_div_round(a: u64, b: u64) -> Option<u64> {
    match b {
        0 => None,
        1 => Some(a),
        // `a / b` is computable and `(a % b) * 2` can not overflow since `b >= 2`. UPDATE <- this is wrong
        b => Some(a / b + if (a % b) * 2 >= b { 1 } else { 0 }),
    }
}

Generates two subsequent labels and an unnecessary jmp instruction:

checked_div_round:
        test    rsi, rsi
        je      .LBB0_1
        cmp     rsi, 1
        je      .LBB0_3
        mov     rax, rdi
        xor     edx, edx
        div     rsi
        add     rdx, rdx
        cmp     rdx, rsi
        mov     rdi, rax
        sbb     rdi, -1
.LBB0_3:
        mov     esi, 1
        jmp     .LBB0_4   ; this jump does not seem necessary since `.LBB0_1` is empty
.LBB0_1:
.LBB0_4:
        mov     rax, rsi
        mov     rdx, rdi
        ret

When compiled with -C opt-level=s using rustc 1.79.0.

Godbolt: https://godbolt.org/z/fhTq3ssnK

P.S. For context, I am not too familiar with asm. I did not find any similar results while searching through the rustc and llvm issue trackers for "double | subsequent | unnecessary label" or "unnecessary jmp | jump". Please let me know how I can improve the quality of my issue reporting.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 17, 2024
@veera-sivarajan
Copy link
Contributor

@rustbot label -needs-triage +C-optimization +T-compiler +A-codegen +A-LLVM

@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-optimization Category: An issue highlighting optimization opportunities or PRs implementing such 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 Jun 17, 2024
@DianQK
Copy link
Member

DianQK commented Jun 18, 2024

Here are multiple solutions to this problem. It can be resolved by directly optimizing in IR (llvm/llvm-project#95919) or during the code generation phase (llvm/llvm-project#95921). I believe the code generation phase is the key point, and we can use undef to eliminate this branch. However, I am more familiar with IR-related aspects, so let me resolve it here now.

@DianQK
Copy link
Member

DianQK commented Jul 9, 2024

Fixed by llvm/llvm-project#95919.
@rustbot label +llvm-fixed-upstream
@rustbot claim

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade label Jul 9, 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 labels Aug 1, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 4, 2024
Add a set of tests for LLVM 19

Close rust-lang#107681. Close rust-lang#118306. Close rust-lang#126585.

r? compiler

try-job: i686-msvc
try-job: arm-android
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2024
Add a set of tests for LLVM 19

Close rust-lang#107681. Close rust-lang#118306. Close rust-lang#126585.

r? compiler

try-job: i686-msvc
try-job: arm-android
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2024
Add a set of tests for LLVM 19

Close rust-lang#107681. Close rust-lang#118306. Close rust-lang#126585.

r? compiler

try-job: i686-msvc
try-job: arm-android
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2024
Add a set of tests for LLVM 19

Close rust-lang#107681. Close rust-lang#118306. Close rust-lang#126585.

r? compiler

try-job: i686-msvc
try-job: arm-android
try-job: test-various
@Skgland
Copy link
Contributor

Skgland commented Aug 6, 2024

I just stumbled past this and am confused by the comment

(a % b) * 2 can not overflow since b >= 2

I would expect this to overflow if a > u64::MAX / 2 and b > a (playground)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 9, 2024
@mickvangelderen
Copy link
Contributor Author

I would expect this to overflow if a > u64::MAX / 2 and b > a (playground)

You're right, I made a logical error.

@bors bors closed this as completed in 69b380d Aug 10, 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-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. 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