-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
match lowering: expand or-candidates mixed with candidates above #126553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Jun 16, 2024
Nadrieril
force-pushed
the
expand-or-pat-into-above
branch
from
June 16, 2024 15:43
4e91b2d
to
4f5f13f
Compare
This comment has been minimized.
This comment has been minimized.
We can't mix them with candidates below them, but we can mix them with candidates above.
Nadrieril
force-pushed
the
expand-or-pat-into-above
branch
from
June 16, 2024 16:41
4f5f13f
to
7b764be
Compare
@bors r+ |
bors
added
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Jun 18, 2024
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this pull request
Jun 19, 2024
…, r=matthewjasper match lowering: expand or-candidates mixed with candidates above This PR tweaks match lowering of or-patterns. Consider this: ```rust match (x, y) { (1, true) => 1, (2, false) => 2, (1 | 2, true | false) => 3, (3 | 4, true | false) => 4, _ => 5, } ``` One might hope that this can be compiled to a single `SwitchInt` on `x` followed by some boolean checks. Before this PR, we compile this to 3 `SwitchInt`s on `x`, because an arm that contains more than one or-pattern was compiled on its own. This PR groups branch `3` with the two branches above, getting us down to 2 `SwitchInt`s on `x`. We can't in general expand or-patterns freely, because this interacts poorly with another optimization we do: or-pattern simplification. When an or-pattern doesn't involve bindings, we branch the success paths of all its alternatives to the same block. The drawback is that in a case like: ```rust match (1, true) { (1 | 2, false) => unreachable!(), (2, _) => unreachable!(), _ => {} } ``` if we used a single `SwitchInt`, by the time we test `false` we don't know whether we came from the `1` case or the `2` case, so we don't know where to go if `false` doesn't match. Hence the limitation: we can process or-pattern alternatives alongside candidates that precede it, but not candidates that follow it. (Unless the or-pattern is the only remaining match pair of its candidate, in which case we can process it alongside whatever). This PR allows the processing of or-pattern alternatives alongside candidates that precede it. One benefit is that we now process or-patterns in a single place in `mod.rs`. r? `@matthewjasper`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 19, 2024
Rollup of 7 pull requests Successful merges: - rust-lang#123782 (Test that opaque types can't have themselves as a hidden type with incompatible lifetimes) - rust-lang#124580 (Suggest removing unused tuple fields if they are the last fields) - rust-lang#125787 (Migrate `bin-emit-no-symbols` `run-make` test to `rmake`) - rust-lang#126553 (match lowering: expand or-candidates mixed with candidates above) - rust-lang#126594 (Make async drop code more consistent with regular drop code) - rust-lang#126654 (Make pretty printing for `f16` and `f128` consistent) - rust-lang#126656 (rustc_type_ir: Omit some struct fields from Debug output) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 19, 2024
Rollup merge of rust-lang#126553 - Nadrieril:expand-or-pat-into-above, r=matthewjasper match lowering: expand or-candidates mixed with candidates above This PR tweaks match lowering of or-patterns. Consider this: ```rust match (x, y) { (1, true) => 1, (2, false) => 2, (1 | 2, true | false) => 3, (3 | 4, true | false) => 4, _ => 5, } ``` One might hope that this can be compiled to a single `SwitchInt` on `x` followed by some boolean checks. Before this PR, we compile this to 3 `SwitchInt`s on `x`, because an arm that contains more than one or-pattern was compiled on its own. This PR groups branch `3` with the two branches above, getting us down to 2 `SwitchInt`s on `x`. We can't in general expand or-patterns freely, because this interacts poorly with another optimization we do: or-pattern simplification. When an or-pattern doesn't involve bindings, we branch the success paths of all its alternatives to the same block. The drawback is that in a case like: ```rust match (1, true) { (1 | 2, false) => unreachable!(), (2, _) => unreachable!(), _ => {} } ``` if we used a single `SwitchInt`, by the time we test `false` we don't know whether we came from the `1` case or the `2` case, so we don't know where to go if `false` doesn't match. Hence the limitation: we can process or-pattern alternatives alongside candidates that precede it, but not candidates that follow it. (Unless the or-pattern is the only remaining match pair of its candidate, in which case we can process it alongside whatever). This PR allows the processing of or-pattern alternatives alongside candidates that precede it. One benefit is that we now process or-patterns in a single place in `mod.rs`. r? ``@matthewjasper``
This was referenced Jun 27, 2024
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 9, 2024
…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 🙏
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 9, 2024
…tthewjasper 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 🙏
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this pull request
Jul 11, 2024
Fix regression in the MIR lowering of or-patterns In rust-lang/rust#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 🙏
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 12, 2024
…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`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 12, 2024
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`
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jul 16, 2024
Fix regression in the MIR lowering of or-patterns In rust-lang/rust#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 🙏
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tweaks match lowering of or-patterns. Consider this:
One might hope that this can be compiled to a single
SwitchInt
onx
followed by some boolean checks. Before this PR, we compile this to 3SwitchInt
s onx
, because an arm that contains more than one or-pattern was compiled on its own. This PR groups branch3
with the two branches above, getting us down to 2SwitchInt
s onx
.We can't in general expand or-patterns freely, because this interacts poorly with another optimization we do: or-pattern simplification. When an or-pattern doesn't involve bindings, we branch the success paths of all its alternatives to the same block. The drawback is that in a case like:
if we used a single
SwitchInt
, by the time we testfalse
we don't know whether we came from the1
case or the2
case, so we don't know where to go iffalse
doesn't match.Hence the limitation: we can process or-pattern alternatives alongside candidates that precede it, but not candidates that follow it. (Unless the or-pattern is the only remaining match pair of its candidate, in which case we can process it alongside whatever).
This PR allows the processing of or-pattern alternatives alongside candidates that precede it. One benefit is that we now process or-patterns in a single place in
mod.rs
.r? @matthewjasper