-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Comments
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. |
Using 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 |
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 |
This issue appears to be fixed in Rust 1.72.0. |
Thank you! Closing this. |
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:
godbolt: https://godbolt.org/z/xjhMq3TrY
But when adding an if+panic the bounds checks get removed: (even though the assume is obvious)
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)The text was updated successfully, but these errors were encountered: