Rewrite a few manual index loops with while-let #73910
Conversation
There were a few instances of this pattern:
```rust
while index < vec.len() {
let item = &vec[index];
// ...
}
```
These can be indexed at once:
```rust
while let Some(item) = vec.get(index) {
// ...
}
```
Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 47425e4 with merge a152931374175aa20d366c38bebdf75643c46443... |
|
I generally like to change the code this way, it's shorter, cleaner, and it removes a potential panic from the binary. But I've seen that performing this change inside some inner loops worsens the code performance :-/ |
|
💥 Test timed out |
|
@bors try |
|
⌛ Trying commit 47425e4 with merge c08b67a4d251676f43286dd6186841233746e0d2... |
|
@leonardo-m I get that it can be fickle, but in this case it specifically made performance better, at least on LLVM 11. AFAICS it's neutral to LLVM 10, but we'll see what perf says. |
|
Might be good to try and file an LLVM bug regardless - seems like knowing that the index is in bounds would probably be fairly common. |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued c08b67a4d251676f43286dd6186841233746e0d2 with parent d462551, future comparison URL. |
Actually, it might be an improvement. The difference I saw in #73526 (comment) was some extra setup computing the bounds, but there was still a branch calling
OK, if I can distill that code to a reasonably small chunk of LLVM IR as a reproducer, then I'll file it. |
|
Finished benchmarking try commit (c08b67a4d251676f43286dd6186841233746e0d2): comparison url. |
|
perf looks clean or like a slight (up to 0.5% improvement), and since it's an improvement in the code, too, I see no reason not to do this. @bors r+ |
|
📌 Commit 47425e4 has been approved by |
bors
added
S-waiting-on-bors
Rewrite a few manual index loops with while-let
There were a few instances of this pattern:
```rust
while index < vec.len() {
let item = &vec[index];
// ...
}
```
These can be indexed at once:
```rust
while let Some(item) = vec.get(index) {
// ...
}
```
Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
Rewrite a few manual index loops with while-let
There were a few instances of this pattern:
```rust
while index < vec.len() {
let item = &vec[index];
// ...
}
```
These can be indexed at once:
```rust
while let Some(item) = vec.get(index) {
// ...
}
```
Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
Rewrite a few manual index loops with while-let
There were a few instances of this pattern:
```rust
while index < vec.len() {
let item = &vec[index];
// ...
}
```
These can be indexed at once:
```rust
while let Some(item) = vec.get(index) {
// ...
}
```
Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
…arth Rollup of 16 pull requests Successful merges: - rust-lang#72569 (Remove legacy InnoSetup GUI installer) - rust-lang#73306 (Don't implement Fn* traits for #[target_feature] functions) - rust-lang#73345 (expand: Stop using nonterminals for passing tokens to attribute and derive macros) - rust-lang#73449 (Provide more information on duplicate lang item error.) - rust-lang#73569 (Handle `macro_rules!` tokens consistently across crates) - rust-lang#73803 (Recover extra trailing angle brackets in struct definition) - rust-lang#73839 (Split and expand nonstandard-style lints unicode unit test.) - rust-lang#73841 (Remove defunct `-Z print-region-graph`) - rust-lang#73848 (Fix markdown rendering in librustc_lexer docs) - rust-lang#73865 (Fix Zulip topic format) - rust-lang#73892 (Clean up E0712 explanation) - rust-lang#73898 (remove duplicate test for rust-lang#61935) - rust-lang#73906 (Add missing backtick in `ty_error_with_message`) - rust-lang#73909 (`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs) - rust-lang#73910 (Rewrite a few manual index loops with while-let) - rust-lang#73929 (Fix comment typo) Failed merges: r? @ghost
It seems highly dependent on ThinLTO, which is going to make a reproducer harder. With codegen-units=1, this bounds check is totally optimized away in both LLVM 10 and 11, even before this PR. I'll poke a little more at it, but I'm not sure how to present such issues to LLVM. |
There were a few instances of this pattern:
These can be indexed at once:
Particularly in
ObligationForest::process_obligations, this mitigatesa codegen regression found with LLVM 11 (#73526).