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: _ => 0 generates worse code than 0 => 0, _ => unreachable!() #118306

Closed
mcy opened this issue Nov 26, 2023 · 14 comments · Fixed by #128584
Closed

Missed optimization: _ => 0 generates worse code than 0 => 0, _ => unreachable!() #118306

mcy opened this issue Nov 26, 2023 · 14 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. A-mir-opt Area: MIR optimizations E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. 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

@mcy
Copy link
Contributor

mcy commented Nov 26, 2023

Consider the following functions (https://godbolt.org/z/a8r3Tc7TE):

pub fn faster(input: u64) -> u64 {
  match input % 4 {
    0 => 0,
    1 | 2 => 1,
    3 => 2,
    _ => unreachable!(),
  }
}

pub fn branchy(input: u64) -> u64 {
  match input % 4 {
    1 | 2 => 1,
    3 => 2,
    _ => 0,
  }
}

These functions have identical behavior: they map input to input % 4 - (input % 4 / 2). In the former case, LLVM generates a nice lookup table for us, but in the latter, it emits an extra branch. The only difference is that I've used _ => ... to avoid needing to write an unreachable-by-optimization branch.

If we look at the generated IR (after -Cpasses=strip,mem2reg,simplifycfg):

define i64 @faster(i64 %0) unnamed_addr #0 {
  %2 = urem i64 %0, 4
  switch i64 %2, label %.unreachabledefault [
    i64 0, label %5
    i64 1, label %3
    i64 2, label %3
    i64 3, label %4
  ]

.unreachabledefault:                              ; preds = %1
  unreachable

3:                                                ; preds = %1, %1
  br label %5

4:                                                ; preds = %1
  br label %5

5:                                                ; preds = %1, %4, %3
  %.0 = phi i64 [ 2, %4 ], [ 1, %3 ], [ 0, %1 ]
  ret i64 %.0
}

define i64 @branchy(i64 %0) unnamed_addr #0 {
  %2 = urem i64 %0, 4
  switch i64 %2, label %5 [
    i64 1, label %3
    i64 2, label %3
    i64 3, label %4
  ]

3:                                                ; preds = %1, %1
  br label %5

4:                                                ; preds = %1
  br label %5

5:                                                ; preds = %1, %4, %3
  %.0 = phi i64 [ 2, %4 ], [ 1, %3 ], [ 0, %1 ]
  ret i64 %.0
}

The problem is clear: LLVM does not seem to realize that it can trivially transform branchy to faster here, by observing that the default in the switch is only taken when %2 == 0.

I suspect this is more LLVM bug than Rust bug, but it feels fixable by a MIR peephole optimization? Unclear. The _ => 0 code I wrote is an attractive nuisance that I imagine other people writing, too, so perhaps there is value to seeing if this optimization can be made before LLVM.

This bug is also present in Clang, in case someone wants to file an LLVM bug: https://godbolt.org/z/x7rec97E7. It's unclear to me if this is the sort of optimization Clang would do in the frontend instead of in LLVM; could go either-or here, tbh.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 26, 2023
@fmease fmease added 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. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation A-mir-opt Area: MIR optimizations and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 26, 2023
@DianQK
Copy link
Member

DianQK commented Nov 26, 2023

Upstream issue: llvm/llvm-project#73446.

@rustbot claim

@DianQK
Copy link
Member

DianQK commented Jan 3, 2024

@rustbot label llvm-fixed-upstream

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade label Jan 3, 2024
@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 Feb 14, 2024
@nikic
Copy link
Contributor

nikic commented Feb 14, 2024

It looks like this is fixed since 1.75, but I don't know what fixed it: https://godbolt.org/z/eGnWbxbG4

@DianQK
Copy link
Member

DianQK commented Feb 20, 2024

I don't think it has been fixed. It looks like the function is not submitted: https://godbolt.org/z/YdqWq8hbb.
Bisected to 5d5edf0? cc @saethlin

@saethlin
Copy link
Member

You need to stick #[inline(never)] or #[no_mangle] on the function otherwise when CE compiles the crate the function is only lowered to MIR as if it has the #[inline] attribute.

@DianQK
Copy link
Member

DianQK commented Feb 20, 2024

You need to stick #[inline(never)] or #[no_mangle] on the function otherwise when CE compiles the crate the function is only lowered to MIR as if it has the #[inline] attribute.

Normally I would do this. Maybe we should mention this somewhere to avoid submitting invalid code to godbolt?

@nikic nikic removed the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Feb 20, 2024
@nikic
Copy link
Contributor

nikic commented Feb 20, 2024

Oops, sorry. I saw that one function was generated and assumed the other one got merged...

@blyxyas
Copy link
Member

blyxyas commented May 23, 2024

Seems like this has been reverted :/, look at the generated assembly with Copt-level=3

@DianQK
Copy link
Member

DianQK commented May 23, 2024

Seems like this has been reverted :/, look at the generated assembly with Copt-level=3

Can you explain why you think that? I don't see any changes: https://godbolt.org/z/rrb5oKbjb.

BTW, I can reland the upstream patch now.

@blyxyas
Copy link
Member

blyxyas commented May 23, 2024

I think that it's been reverted because the assembly output contains this:

faster:
        and     edi, 3
        lea     rax, [rip + .Lswitch.table.faster]
        mov     rax, qword ptr [rax + 8*rdi]
        ret

branchy:
        and     edi, 3
        lea     rax, [rdi - 1]
        cmp     rax, 2
        ja      .LBB1_1
        lea     rax, [rip + .Lswitch.table.branchy]
        mov     rax, qword ptr [rax + 8*rdi - 8]
        ret

The branchy branch still has some branching (this can also be seen on the LLVM IR, branchy has 12 lines and 2 br jumps, while faster has four lines and no branches)

@DianQK
Copy link
Member

DianQK commented May 24, 2024

Ah, I think you're saying that this optimization is still in a missing state, right?

@blyxyas
Copy link
Member

blyxyas commented May 24, 2024

Yep, I thought you meant that you implemented the optimization, is it not implemented? 😅

@DianQK
Copy link
Member

DianQK commented May 25, 2024

Yep, I thought you meant that you implemented the optimization, is it not implemented? 😅

Yes, I have implemented it, but due to the compilation time issue mentioned in llvm/llvm-project#78578, I had to revert the commit. Now I have relanded it: llvm/llvm-project#73446 (comment).

@rustbot label +llvm-fixed-upstream

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade label May 25, 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
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
@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. A-mir-opt Area: MIR optimizations E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. 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.

7 participants