One more missed bounds check optimization #75525
Comments
This comment has been minimized.
This comment has been minimized.
rustbot
added
A-LLVM
|
This could be improved by optimizing MIR. But the problem rely on LLVM too. |
|
Same as #73396? |
|
Posted on llvm bugzilla: |
|
Thanks for that report! I've played some more with my examples and these three cases are particularly interesting for me: https://godbolt.org/z/z6o3Pc |
|
Since Clang does no optimizations, this is all about about LLVM:) Also worth to see: |
Eliminate some other bound checks when index comes from an enum rust-lang#36962 introduced an assumption for the upper limit of the enum's value. This PR adds an assumption to the lower value as well. I've modified the original codegen test to show that derived (in that case, adding 1) values also don't generate bounds checks. However, this test is actually carefully crafted to not hit a bug: if the enum's variants are modified to 1 and 2 instead of 2 and 3, the test fails by adding a bounds check. I suppose this is an LLVM issue and rust-lang#75525, while not exactly in this context should be tracking it. I'm not at all confident if this patch can be accepted, or even if it _should_ be accepted in this state. But I'm curious about what others think :) ~Improves~ Should improve rust-lang#13926 but does not close it because it's not exactly predictable, where bounds checks may pop up against the assumptions.
|
This happens because LLVM canonicalizes |
|
Partial fix: llvm/llvm-project@445db89 |
|
Partial fix 2: llvm/llvm-project@fe79061 |
|
I confirmed that |
|
This issue has been fixed by the LLVM 12 update (#81451). Note that in the original compiler explorer link, f1 has a bounds check, which is expected there. Other examples are optimized now. |
Add regression test for rust-lang#75525
Add regression test for rust-lang#75525
Rollup of 8 pull requests Successful merges: - rust-lang#81127 (Improve sift_down performance in BinaryHeap) - rust-lang#81879 (Added #[repr(transparent)] to core::cmp::Reverse) - rust-lang#82048 (or-patterns: disallow in `let` bindings) - rust-lang#82731 (Bump libc dependency of std to 0.2.88.) - rust-lang#82799 (Add regression test for rust-lang#75525) - rust-lang#82841 (Change x64 size checks to not apply to x32.) - rust-lang#82883 (Update Cargo) - rust-lang#82887 (Update CONTRIBUTING.md) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
https://godbolt.org/z/3TGbK9
It looks like bounds checks are not eliminated when more than one restriction is applied to the index variable (i.e. when it's range checked from two sides). Late edit: It's also interesting that the issue disappears for certain lower limits. For example
if idx > 2 && idx < 8is optimized, whileif idx > 5 && idx < 8is not.Using assumptions, the bounds checks are removed as expected: https://godbolt.org/z/Ksn8dW
However, if any of the assumptions is a GE or LE instead of a GT or LT, the bounds checks are back: https://godbolt.org/z/YhK1rT
The text was updated successfully, but these errors were encountered: