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: eagerly simplify match pairs #120904

Merged
merged 8 commits into from
Feb 21, 2024

Conversation

Nadrieril
Copy link
Member

This removes one important complication from match lowering. Before this, match pair simplification (which includes collecting bindings and type ascriptions) was intertwined with the whole match lowering algorithm.

I'm avoiding this by storing in each MatchPair the sub-MatchPairs that correspond to its subfields. This makes it possible to simplify everything (except or-patterns) in Candidate::new().

This should open up further simplifications. It will also give us proper control over the order of bindings.

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 Feb 11, 2024
@Nadrieril
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2024
@bors
Copy link
Contributor

bors commented Feb 11, 2024

⌛ Trying commit 45ef29c with merge d39daec...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
…e-repr, r=<try>

match lowering: eagerly simplify match pairs

This removes one important complication from match lowering. Before this, match pair simplification (which includes collecting bindings and type ascriptions) was intertwined with the whole match lowering algorithm.

I'm avoiding this by storing in each `MatchPair` the sub-`MatchPair`s that correspond to its subfields. This makes it possible to simplify everything (except or-patterns) in `Candidate::new()`.

This should open up further simplifications. It will also give us proper control over the order of bindings.

r? `@matthewjasper`
@bors
Copy link
Contributor

bors commented Feb 11, 2024

☀️ Try build successful - checks-actions
Build commit: d39daec (d39daecd2270ce85a3888cadd36b654a8f380a06)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d39daec): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-3.1%, -1.8%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-3.1%, -1.8%] 6

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.6% [3.6%, 3.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.2%] 3
All ❌✅ (primary) - - 0

Cycles

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-3.0%, -2.0%] 5
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.4% [-3.0%, -2.0%] 5

Binary size

Results

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.

mean range count
Regressions ❌
(primary)
0.2% [0.1%, 0.2%] 2
Regressions ❌
(secondary)
0.1% [0.1%, 0.1%] 1
Improvements ✅
(primary)
-0.0% [-0.1%, -0.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [-0.1%, 0.2%] 4

Bootstrap: 666.058s -> 664.547s (-0.23%)
Artifact size: 308.32 MiB -> 308.30 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2024
@Nadrieril Nadrieril force-pushed the match-lowering-intermediate-repr branch 2 times, most recently from 25247a7 to d33fbc9 Compare February 14, 2024 21:50
Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of style/comment nits.

@@ -142,31 +126,31 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// candidate.
fn simplify_match_pair<'pat>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment should be updated for change in parameters

@@ -589,22 +589,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// away.)
let (match_pair_index, match_pair) =
candidate.match_pairs.iter().enumerate().find(|&(_, mp)| mp.place == *test_place)?;
let mut fully_matched = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer this to be let fully_matched; and have every branch assign true or false.

Copy link
Member Author

@Nadrieril Nadrieril Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh yeah, in another branch I in fact forgot to set it once, and the resulting infinite loop crashed my laptop x) Good shout.

@Nadrieril
Copy link
Member Author

Thanks for the review!

@rustbot ready

@Nadrieril Nadrieril force-pushed the match-lowering-intermediate-repr branch from d33fbc9 to 328c776 Compare February 19, 2024 20:36
matthewjasper

This comment was marked as outdated.

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2024

📌 Commit 328c776 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 Feb 20, 2024
@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 Feb 20, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

⌛ Testing commit 328c776 with merge 1aa6816...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
…e-repr, r=matthewjasper

match lowering: eagerly simplify match pairs

This removes one important complication from match lowering. Before this, match pair simplification (which includes collecting bindings and type ascriptions) was intertwined with the whole match lowering algorithm.

I'm avoiding this by storing in each `MatchPair` the sub-`MatchPair`s that correspond to its subfields. This makes it possible to simplify everything (except or-patterns) in `Candidate::new()`.

This should open up further simplifications. It will also give us proper control over the order of bindings.

r? `@matthewjasper`
@bors
Copy link
Contributor

bors commented Feb 20, 2024

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Nadrieril
Copy link
Member Author

I can't tell which is the test that timed out or if this has anything to do with this PR.

@bors retry

@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 Feb 20, 2024
@bors
Copy link
Contributor

bors commented Feb 21, 2024

⌛ Testing commit 328c776 with merge 4e65074...

@bors
Copy link
Contributor

bors commented Feb 21, 2024

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing 4e65074 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 21, 2024
@bors bors merged commit 4e65074 into rust-lang:master Feb 21, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e65074): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 3
Improvements ✅
(primary)
-2.0% [-3.1%, -0.2%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.0% [-3.1%, -0.2%] 7

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.4% [-3.6%, -3.3%] 2
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) -3.4% [-3.6%, -3.3%] 2

Cycles

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-3.0%, -1.6%] 5
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.4% [-3.0%, -1.6%] 5

Binary size

Results

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.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.2%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [0.0%, 0.2%] 3

Bootstrap: 639.89s -> 640.784s (0.14%)
Artifact size: 308.62 MiB -> 308.61 MiB (-0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Feb 21, 2024
@Nadrieril Nadrieril deleted the match-lowering-intermediate-repr branch February 21, 2024 10:19
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 22, 2024
… r=matthewjasper

match lowering: Introduce a `TestCase` enum to replace most matching on `PatKind`

Introduces `TestCase` that represents the specific outcome of a test. It complements `TestKind` which represents a test. In `MatchPair::new()` we select the appropriate `TestCase` for the pattern, and after that we almost never have to inspect the pattern directly during match lowering.

Together with rust-lang#120904, this makes `MatchPair` into a standalone abstraction that hides the details of `thir::Pat`. This will become even truer in the next PR where I make `TestCase` handle or patterns. This opens the door to a lot of future simplifications.

r? `@matthewjasper`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2024
Rollup merge of rust-lang#121393 - Nadrieril:match-lowering-testcase, r=matthewjasper

match lowering: Introduce a `TestCase` enum to replace most matching on `PatKind`

Introduces `TestCase` that represents the specific outcome of a test. It complements `TestKind` which represents a test. In `MatchPair::new()` we select the appropriate `TestCase` for the pattern, and after that we almost never have to inspect the pattern directly during match lowering.

Together with rust-lang#120904, this makes `MatchPair` into a standalone abstraction that hides the details of `thir::Pat`. This will become even truer in the next PR where I make `TestCase` handle or patterns. This opens the door to a lot of future simplifications.

r? `@matthewjasper`
@rylev
Copy link
Member

rylev commented Feb 27, 2024

Improvements far outweigh the regressions so I don't think this needs further investigation.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 27, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Jul 17, 2024
In rust-lang#120904, `MatchPair` became able to store other match pairs as children,
forming a tree. That has made the old name confusing, so this patch renames the
type to `MatchPairTree`.
Zalathar added a commit to Zalathar/rust that referenced this pull request Jul 17, 2024
In rust-lang#120904, `MatchPair` became able to store other match pairs as children,
forming a tree. That has made the old name confusing, so this patch renames the
type to `MatchPairTree`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2024
match lowering: Rename `MatchPair` to `MatchPairTree`

In rust-lang#120904, `MatchPair` became able to store other match pairs as children, forming a tree. That has made the old name confusing, so this patch renames the type to `MatchPairTree`.

This PR also includes a patch renaming the `test` method to `pick_test_for_match_pair`, since it would conflict with the main change.

r? `@Nadrieril`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup merge of rust-lang#127858 - Zalathar:pair-tree, r=Nadrieril

match lowering: Rename `MatchPair` to `MatchPairTree`

In rust-lang#120904, `MatchPair` became able to store other match pairs as children, forming a tree. That has made the old name confusing, so this patch renames the type to `MatchPairTree`.

This PR also includes a patch renaming the `test` method to `pick_test_for_match_pair`, since it would conflict with the main change.

r? `@Nadrieril`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.

7 participants