Skip to content
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
merged 6 commits into from
Jun 19, 2024

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Jun 16, 2024

This PR tweaks match lowering of or-patterns. Consider this:

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 SwitchInts 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 SwitchInts 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:

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

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit 7b764be has been approved by matthewjasper

It is now in the queue for this repository.

@bors 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
@bors bors merged commit e111e99 into rust-lang:master Jun 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
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``
@Nadrieril Nadrieril deleted the expand-or-pat-into-above branch June 19, 2024 12:09
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants