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

Obvious bounds checks don't get optimized unless I add a if+panic #90794

Closed
elichai opened this issue Nov 11, 2021 · 6 comments
Closed

Obvious bounds checks don't get optimized unless I add a if+panic #90794

elichai opened this issue Nov 11, 2021 · 6 comments
Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@elichai
Copy link
Contributor

elichai commented Nov 11, 2021

I'm writing code to rank a matrix and the compiler doesn't see it can remove bounds checks even though it's obvious.
But if I add a check on the index the compiler can then remove the bounds checks.
The minimal code to reproduce it:

pub fn compute_rank(mat_float: [[f64; 64]; 64]) -> usize {
    const EPS: f64 = 1e-9;
    let row_selected = [false; 64];
    for i in 0..64 {
        for j in 0..64 {
            if !row_selected[j] && mat_float[j][i] > EPS {
                break;
            }
        }
    }
    0
}

godbolt: https://godbolt.org/z/xjhMq3TrY

But when adding an if+panic the bounds checks get removed: (even though the assume is obvious)

pub fn compute_rank(mat_float: [[f64; 64]; 64]) -> usize {
    const EPS: f64 = 1e-9;
    let row_selected = [false; 64];
    for i in 0..64 {
        if i >= 64 {panic!("This should never happen")}
        for j in 0..64 {
            if !row_selected[j] && mat_float[j][i] > EPS {
                break;
            }
        }
    }
    0
}

https://godbolt.org/z/Ecvd4e9nM

(The full code is obviously longer and contain more bounds checks that all get eliminated by this check)

(Originally I used unreachable_unchecked() for this but somehow even an if+panic solves this)

@leonardo-m
Copy link

leonardo-m commented Nov 11, 2021

Despite Rust is a system language, its handling of loops is still rather broken. Here LLVM removes all bound tests:

pub fn compute_rank(mat_float: [[f64; 64]; 64]) -> usize {
    const EPS: f64 = 1e-9;
    let row_selected = [false; 64];
    let mut i = 0;
    while i < 64 {
        for j in 0 .. 64 {
            if !row_selected[j] && mat_float[j][i] > EPS {
                break;
            }
        }
        i += 1;
    }
    0
}

Other loop problems (that lead to significant inefficiencies for computational kernels) are when you use ..= instead of .., and when you use a step_by especially when the step value isn't a compile-time constant. So often using a while loop like this is the way to go in reasonably efficient Rust code. And this is sad. C language is better regarding for loops.

@the8472
Copy link
Member

the8472 commented Nov 11, 2021

Using range.for_each instead for _ in range also optimizes better, especially for inclusive ranges.

For inclusive ranges that's because the external iteration version has to keep track of the exhaustion state while the exclusive range can use the end element to indicate exhaustion.

For exclusive ranges it is strange that for_each works better here because there's no dedicated fold impl for exclusive ranges and the default implementation isn't all that different than the for loop desugaring (although less verbose than the latter) so they should optimize the same.

@nikic nikic added I-slow Issue: Problems and improvements with respect to performance of generated code. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Nov 11, 2021
@MSxDOS
Copy link

MSxDOS commented Nov 12, 2021

I also encountered cases where adding an assertion removed both the unnecessary bounds check and the assertion itself, which is kinda weird.

This whole function should be optimized to just return zero anyways, and it is, up to rust 1.54 and only if the i < 64 assertion is present.

@workingjubilee
Copy link
Contributor

Are we sure this is not a duplicate of #30112, #39220, #41789, #51709, #55147, #58388, #71066, #71935, #72462, #80075, #81253, #81432, or #84314? It's fine if it isn't, even, I am just trying to figure out which ones of these are unique.

@Nilstrieb Nilstrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@KamilaBorowska
Copy link
Contributor

This issue appears to be fixed in Rust 1.72.0.

@workingjubilee
Copy link
Contributor

Thank you! Closing this.

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. 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

No branches or pull requests

8 participants