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.

Sign up for GitHub

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

Jump to bottom

More rustc_mir_dataflow cleanups #118638

Merged
merged 4 commits into from
Dec 9, 2023
Merged

More rustc_mir_dataflow cleanups #118638

merged 4 commits into from
Dec 9, 2023

Conversation

nnethercote
Copy link
Contributor

@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 Dec 5, 2023
@nnethercote
Copy link
Contributor Author

The third commit might be controversial, but the Analysis/GenKillAnalysis situation was bugging me, and this was my best attempt to tame it.

cjgillot
cjgillot reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

I disagree with the 3rd commit. All the logic around gen-kill analyses relies on the exact implementation of the blanket impl you remove. With a macro, we do not enforce the implementation any more (client code could avoid using it), that could lead to inconsistencies and bugs.

compiler/rustc_borrowck/src/dataflow.rs Outdated Show resolved Hide resolved
@nnethercote
Remove unused arguments from ResultsVisitor::visit_block_{start,end}.
f312775
@nnethercote
Remove impl_visitable!.
60e7c68 
It is used just once. With it removed, the relevant code is a little
boilerplate-y but much easier to read, and is the same length. Overall I
think it's an improvement.
@nnethercote
Remove BorrowckAnalyses.
0158404 
This results in two non-generic types being used: `BorrowckResults` and
`BorrowckFlowState`. It's a net reduction in lines of code, and a little
easier to read.
@nnethercote
Tweak GenKillAnalysis.
4b364b6 
`GenKillAnalysis` has five methods that take a transfer function arg:
- `statement_effect`
- `before_statement_effect`
- `terminator_effect`
- `before_terminator_effect`
- `call_return_effect`

All the transfer function args have type `&mut impl GenKill<Self::Idx>`,
except for `terminator_effect`, which takes the simpler `Self::Domain`.

But only the first two need to be `impl GenKill`. The other
three can all be `Self::Domain`, just like `Analysis`. So this commit
changes the last two to take `Self::Domain`, making `GenKillAnalysis`
and `Analysis` more similar.

(Another idea would be to make all these methods `impl GenKill`. But
that doesn't work: `MaybeInitializedPlaces::terminator_effect` requires
the arg be `Self::Domain` so that `self_is_unwind_dead(place, state)`
can be called on it.)
@nnethercote nnethercote force-pushed the rustc_mir_dataflow-more branch from db7352d to 4b364b6 Compare December 7, 2023 22:54
@nnethercote
Copy link
Contributor Author

I disagree with the 3rd commit.

Ok, fair enough. I have replaced it with a smaller commit that changes the type of the trans arg in two of the GenKillAnalysis methods. Explanation in the commit log.

@nnethercote
Copy link
Contributor Author

So there are now four commits. The first two are unchanged, the last two are new and need reviewing.

@cjgillot
Copy link
Contributor

cjgillot commented Dec 9, 2023

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2023

📌 Commit 4b364b6 has been approved by cjgillot

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 Dec 9, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
@bors
Auto merge of rust-lang#118780 - GuillaumeGomez:rollup-nd0syaf, r=Gui…
08587a5 
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117953 (Add more SIMD platform-intrinsics)
 - rust-lang#118057 (dedup for duplicate suggestions)
 - rust-lang#118638 (More `rustc_mir_dataflow` cleanups)
 - rust-lang#118702 (Strengthen well known check-cfg names and values test)
 - rust-lang#118734 (Unescaping cleanups)
 - rust-lang#118766 (Lower some forgotten spans)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 546643c into rust-lang:master Dec 9, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 9, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
@rust-timer
Unrolled build for rust-lang#118638
c9bb0b0 
Rollup merge of rust-lang#118638 - nnethercote:rustc_mir_dataflow-more, r=cjgillot

More `rustc_mir_dataflow` cleanups

r? `@cjgillot`
@nnethercote nnethercote deleted the rustc_mir_dataflow-more branch December 10, 2023 21:19
@nnethercote nnethercote mentioned this pull request Oct 23, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2024
@bors
Auto merge of rust-lang#132062 - nnethercote:still-more-rustc_mir_dat…
1c188eb 
…aflow-cleanups, r=<try>

Still more `rustc_mir_dataflow` cleanups

A sequel to rust-lang#118203, rust-lang#118230, rust-lang#118638, and rust-lang#131481.

I'm pleased with this PR. It cleans up a few things that have been bugging me for a while. It took quite some effort to find these improvements.

r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

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
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nnethercote @cjgillot @bors @rustbot