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

mir_build: Integrate "simplification" steps into match-pair-tree creation #137875

Merged
merged 6 commits into from
Mar 5, 2025

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Mar 2, 2025

The “simplification” step helps to prepare THIR patterns for lowering into MIR, and originally dates back to the earliest days of MIR in the compiler.

Over time, various intermediate data structures have been introduced (e.g. MatchPair, later renamed to MatchPairTree) that reduce the need for a separate simplification step, because some of the necessary simplifications can be built into the construction of those intermediate structures instead. This PR continues that process to its logical conclusion and removes the simplification step entirely, by integrating its remaining responsibilities into match-pair-tree creation: flattening “irrefutable” nodes, collecting bindings/ascriptions in flat lists, and sorting or-patterns after other subpatterns.

This has a few immediate benefits:

  • We can remove TestCase::Irrefutable, which was not allowed to exist after simplification, and was much larger than other test-case variants.
  • We can make MatchPairTree::place non-optional, because only irrefutable nodes could fail to have a place.

In the future, this should also help with some ideas I have for simplifying how AscribeUserType and ExpandedConstant nodes are handled, by representing them as side-data keyed by THIR pattern ID, so that they are no longer their own kinds of THIR pattern node.

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Mar 2, 2025
@Zalathar
Copy link
Contributor Author

Zalathar commented Mar 2, 2025

cc @Nadrieril

(Looks like I accidentally broke triagebot pings for rustc_mir_build/src/builder/matches; there's a fix at #137876.)

@Zalathar
Copy link
Contributor Author

Zalathar commented Mar 2, 2025

Actually, this seems more appropriate:

r? Nadrieril

@rustbot rustbot assigned Nadrieril and unassigned Noratrieb Mar 2, 2025
Comment on lines 1273 to 1271
///
/// Invariant: Or-patterns must be sorted to the end.
subpairs: Vec<Self>,
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to remove this: the invariant is relevant for Candidates only; adding it here is potentially confusing. We could even remove it from FlatPat if we make sure to sort in the FlatPat -> Candidate conversion step.

Copy link
Member

@Nadrieril Nadrieril left a comment

Choose a reason for hiding this comment

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

I like this, anything that removes some complexity in matchy lowering is a success. I left two requests for changes.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2025

Some changes occurred in match lowering

cc @Nadrieril

Zalathar added 6 commits March 5, 2025 23:25
What remained of this simplification process has been integrated into
construction of the match-pair trees.
As the invariant indicated, this place could only be none for
`TestCase::Irrefutable` nodes, which no longer exist.
@Zalathar
Copy link
Contributor Author

Zalathar commented Mar 5, 2025

OK, I rebased and then addressed the feedback (diff).

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2025
@Nadrieril
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2025

📌 Commit e3e74bc has been approved by Nadrieril

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2025
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 5, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2025
Rollup of 20 pull requests

Successful merges:

 - rust-lang#134063 (dec2flt: Clean up float parsing modules)
 - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra)
 - rust-lang#136662 (Count char width at most once in `Formatter::pad`)
 - rust-lang#136764 (Make `ptr_cast_add_auto_to_object` lint into hard error)
 - rust-lang#136798 (Added documentation for flushing per rust-lang#74348)
 - rust-lang#136865 (Perform deeper compiletest path normalization for `$TEST_BUILD_DIR` to account for compare-mode/debugger cases, and normalize long type file filename hashes)
 - rust-lang#136975 (Look for `python3` first on MacOS, not `py`)
 - rust-lang#136977 (Upload Datadog metrics with citool)
 - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs)
 - rust-lang#137298 (Check signature WF when lowering MIR body)
 - rust-lang#137463 ([illumos] attempt to use posix_spawn to spawn processes)
 - rust-lang#137477 (uefi: Add Service Binding Protocol abstraction)
 - rust-lang#137569 (Stabilize `string_extend_from_within`)
 - rust-lang#137633 (Only use implied bounds hack if bevy, and use deeply normalize in implied bounds hack)
 - rust-lang#137679 (Various coretests improvements)
 - rust-lang#137723 (Make `rust.description` more general-purpose and pass `CFG_VER_DESCRIPTION`)
 - rust-lang#137728 (Remove unsizing coercions for tuples)
 - rust-lang#137731 (Resume one waiter at once in deadlock handler)
 - rust-lang#137875 (mir_build: Integrate "simplification" steps into match-pair-tree creation)
 - rust-lang#138028 (compiler: add `ExternAbi::is_rustic_abi`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9d1b2f7 into rust-lang:master Mar 5, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 5, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2025
Rollup merge of rust-lang#137875 - Zalathar:irrefutable, r=Nadrieril

mir_build: Integrate "simplification" steps into match-pair-tree creation

The “simplification” step helps to prepare THIR patterns for lowering into MIR, and originally dates back to the earliest days of MIR in the compiler.

Over time, various intermediate data structures have been introduced (e.g. `MatchPair`, later renamed to `MatchPairTree`) that reduce the need for a separate simplification step, because some of the necessary simplifications can be built into the construction of those intermediate structures instead. This PR continues that process to its logical conclusion and removes the simplification step entirely, by integrating its remaining responsibilities into match-pair-tree creation: flattening “irrefutable” nodes, collecting bindings/ascriptions in flat lists, and sorting or-patterns after other subpatterns.

This has a few immediate benefits:
- We can remove `TestCase::Irrefutable`, which was not allowed to exist after simplification, and was much larger than other test-case variants.
- We can make `MatchPairTree::place` non-optional, because only irrefutable nodes could fail to have a place.

In the future, this should also help with some ideas I have for simplifying how `AscribeUserType` and `ExpandedConstant` nodes are handled, by representing them as side-data keyed by THIR pattern ID, so that they are no longer their own kinds of THIR pattern node.
@Zalathar Zalathar deleted the irrefutable branch March 6, 2025 00:04
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