-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix regression in the MIR lowering of or-patterns #127028
Conversation
if expand_until != 0 { | ||
expand_until = i + 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was "include as many candidates until we find a candidate where we should stop" but I forgot to keep incrementing the index so I cut off candidates I should have included. As you can see in the test file, this created unnecessary extra SwitchInt
s.
r? @matthewjasper |
…matthewjasper Fix regression in the MIR lowering of or-patterns In rust-lang#126553 I made a silly indexing mistake and regressed the MIR lowering of or-patterns. This fixes it. r? `@compiler-errors` because I'd like this to be merged quickly 🙏
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127028 (Fix regression in the MIR lowering of or-patterns) - rust-lang#127091 (impl FusedIterator and a size hint for the error sources iter) - rust-lang#127358 (Automatically taint when reporting errors from ItemCtxt) - rust-lang#127382 (Use verbose style when suggesting changing `const` with `let`) - rust-lang#127397 (fix interleaved output in the default panic hook when multiple threads panic simultaneously) - rust-lang#127484 (`#[doc(alias)]`'s doc: say that ASCII spaces are allowed) - rust-lang#127496 (Update `f16`/`f128` FIXMEs that needed `(NEG_)INFINITY`) - rust-lang#127508 (small search graph refactor) r? `@ghost` `@rustbot` modify labels: rollup
☀️ Test successful - checks-actions |
Ty! |
Finished benchmarking commit (9dcaa7f): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 0.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.1%, secondary -2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 701.812s -> 702.397s (0.08%) |
…atthewjasper match lowering: Clarify the main loop of the algorithm Now that we expand or-patterns in a single place in the algorithm, we can move it (back) to the main part of the loop. This makes the call-graph of the main loop rather simple: `match_candidates` has three branches that each call back to `match_candidates`. The remaining tricky part is `finalize_or_candidate`. I also factored out the whole "process a prefix of the candidates then process the rest" thing which I think helps legibility. The first two commits are a fix for an indexing mistake I introduced in rust-lang#126553, already sumitted in rust-lang#127028 but feel free to merge this first. r? `@matthewjasper`
Rollup merge of rust-lang#127164 - Nadrieril:clean-lowering-loop, r=matthewjasper match lowering: Clarify the main loop of the algorithm Now that we expand or-patterns in a single place in the algorithm, we can move it (back) to the main part of the loop. This makes the call-graph of the main loop rather simple: `match_candidates` has three branches that each call back to `match_candidates`. The remaining tricky part is `finalize_or_candidate`. I also factored out the whole "process a prefix of the candidates then process the rest" thing which I think helps legibility. The first two commits are a fix for an indexing mistake I introduced in rust-lang#126553, already sumitted in rust-lang#127028 but feel free to merge this first. r? `@matthewjasper`
Given this is a correctness fix, and the regressions seem in line with previous historical performance (i.e., it regressed back to a previous baseline), I think it's safe to call this triaged. Let me know if anyone disagrees. @rustbot label: +perf-regression-triaged |
In #126553 I made a silly indexing mistake and regressed the MIR lowering of or-patterns. This fixes it.
r? @compiler-errors because I'd like this to be merged quickly 🙏