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

[ER] A missed removal of division by zero test #86109

Closed
leonardo-m opened this issue Jun 7, 2021 · 7 comments · Fixed by #132170
Closed

[ER] A missed removal of division by zero test #86109

leonardo-m opened this issue Jun 7, 2021 · 7 comments · Fixed by #132170
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

@leonardo-m
Copy link

This function foo:

type T = i16;

pub fn foo(start: T) -> T {
    if start <= 0 { return 0; }
    let mut count = 0;
    for i in start .. 10_000 {
        if 752 % i != 0 {
            count += 1;
        }
    }
    count
}

fn main() {
    println!("{}", (T::MIN ..= T::MAX)
                   .map(foo)
                   .sum::<T>());
}

Compiled with optimizations by rustc (1.54.0-nightly 4de7572 2021-05-01) gives:

foo:
        lea     eax, [rdi - 1]
        movzx   eax, ax
        cmp     eax, 9998
        ja      .LBB0_1
        mov     r8d, 9999
        sub     r8d, edi
        mov     esi, edi
        neg     esi
        xor     ecx, ecx
.LBB0_4:
        cmp     r8w, si
        jae     .LBB0_6
        mov     ax, 752
        xor     edx, edx
        idiv    di
        lea     eax, [rdi + 1]
        cmp     dx, 1
        sbb     cx, -1
        movzx   edx, ax
        mov     edi, eax
        cmp     edx, 10000
        jne     .LBB0_4
        mov     eax, ecx
        ret
.LBB0_1:
        xor     ecx, ecx
        mov     eax, ecx
        ret
.LBB0_6:
        push    rax
        lea     rdi, [rip + str.0]
        lea     rdx, [rip + .L__unnamed_1]
        mov     esi, 57
        call    qword ptr [rip + core::panicking::panic@GOTPCREL]
        ud2

str.0:
        .ascii  "attempt to calculate the remainder with a divisor of zero"

Running the code you confirm that the divisor can't be zero.
I think Rustc should be able to remove this test.
(The same code with T=u16 contains no division by zero test).

@inquisitivecrystal
Copy link
Contributor

@rustbot label A-codegen C-enhancement I-slow +T-compiler

@rustbot rustbot added A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. 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. labels Jun 8, 2021
@klensy
Copy link
Contributor

klensy commented Jan 31, 2022

Still an issue with 1.60.0-nightly (08df8b8 2022-01-30).

I think this is similar to the same problem with (missed) optimizations, when we match on some variable and expect compiler to match on values of that variable, not on it's type.

For provided example we can cast i16 to u16 after check, so panic go away:

pub fn foo(start: T) -> T {
    if start <= 0 { return 0; }
    let start = start as u16; // add this to change type
    let mut count = 0;
    ...

@leonardo-m
Copy link
Author

Note that with rustc 1.60.0-nightly (08df8b8 2022-01-30) replacing in my original foo code the "for i in start .. 10_000 {" with an equivalent while loop is enough to remove the dividend test.

@klensy
Copy link
Contributor

klensy commented Jan 31, 2022

Note that with rustc 1.60.0-nightly (08df8b8 2022-01-30) replacing in my original foo code the "for i in start .. 10_000 {" with an equivalent while loop is enough to remove the dividend test.

That works for 1.54 too.

@jieyouxu
Copy link
Member

Triage: for the example in the issue description, division by zero was present in 1.69 but disappeared in 1.70.

@leonardo-m
Copy link
Author

Right, closed.

@jieyouxu
Copy link
Member

Sorry, I was reviewing the PR that's adding a regression test for this, we should close this issue after the regression test lands

@jieyouxu jieyouxu reopened this Oct 26, 2024
@jieyouxu jieyouxu added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 26, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 27, 2024
…ieyouxu

Add a Few Codegen Tests

Closes rust-lang#86109
Closes rust-lang#64219

Those issues somehow got fixed over time.

So, this PR adds a couple of codegen tests to ensure we don't regress in the future.
@bors bors closed this as completed in a513e0e Nov 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2024
Rollup merge of rust-lang#132170 - veera-sivarajan:codegen-tests, r=jieyouxu

Add a Few Codegen Tests

Closes rust-lang#86109
Closes rust-lang#64219

Those issues somehow got fixed over time.

So, this PR adds a couple of codegen tests to ensure we don't regress in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.

5 participants