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 .rem_euclid(2^n) on signed integer types. #115855

Closed
HypheX opened this issue Sep 14, 2023 · 5 comments
Closed

Missed optimization for .rem_euclid(2^n) on signed integer types. #115855

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

Comments

@HypheX
Copy link

HypheX commented Sep 14, 2023

When calling .rem_euclid(n) on a signed integer type, it fails to apply the optimization for powers of 2, where it should do a bitwise AND operation instead.

Missed optimization

pub fn rem(n: i32) -> i32 {
    n.rem_euclid(2)
}
example::rem:
        mov     eax, edi
        shr     eax, 31
        add     eax, edi
        and     eax, -2
        sub     edi, eax
        mov     eax, 1
        cmovns  eax, edi
        ret

How it should be optimized

pub fn rem(n: i32) -> i32 {
    n & 1
}
example::rem:
        mov     eax, edi
        and     eax, 1
        ret
Testing the optimization:

n = 2

?play

let mut passed = true;
for i in (std::i16::MIN..=std::i16::MAX) {
    if (i&1) != (i.rem_euclid(2)) {
        passed = false;
        break
    }
}
println!("{passed}");
true

n = 1

?play

let mut passed = true;
for i in (std::i16::MIN..=std::i16::MAX) {
    if (i&0) != (i.rem_euclid(1)) {
        passed = false;
        break
    }
}
println!("{passed}");
true

n = 4

?play

let mut passed = true;
for i in (std::i16::MIN..=std::i16::MAX) {
    if (i&3) != (i.rem_euclid(4)) {
        passed = false;
        break
    }
}
println!("{passed}");
true

Curiously, the optimization is applied to unsigned integers just fine

pub fn rem(n: u32) -> u32 {
    n.rem_euclid(2) 
}
example::rem:
        mov     eax, edi
        and     eax, 1
        ret
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 14, 2023
@nikic nikic 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. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 14, 2023
@nikic
Copy link
Contributor

nikic commented Sep 15, 2023

If you are going to file an upstream issue yourself, please do link back to it so other people don't do duplicate work: llvm/llvm-project#66417

Anyway, this issue should already be fixed in LLVM 18.

@nikic nikic added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade label Sep 15, 2023
@fmease
Copy link
Member

fmease commented Sep 15, 2023

As an aside, the title should be 2^n (powers of two) instead of n^2 (squares).

@HypheX HypheX changed the title Missed optimization for .rem_euclid(n^2) on signed integer types. Missed optimization for .rem_euclid(2^n) on signed integer types. Sep 15, 2023
@HypheX
Copy link
Author

HypheX commented Sep 15, 2023

As an aside, the title should be 2^n (powers of two) instead of n^2 (squares).

Thanks, wow, had a brainfart there.

@nikic
Copy link
Contributor

nikic commented Feb 14, 2024

Confirmed fixed by #120055, needs codegen test. Godbolt: https://rust.godbolt.org/z/cf3Ej1Mq3

@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
@HypheX
Copy link
Author

HypheX commented May 10, 2024

Fixed in 1.78.0 (Updated to LLVM 18)

@HypheX HypheX closed this as completed May 10, 2024
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. 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.
Projects
None yet
Development

No branches or pull requests

4 participants