Skip to content

Comments

dump_mir cleanups#145421

Merged
bors merged 6 commits intorust-lang:masterfrom
nnethercote:dump_mir-cleanups
Sep 1, 2025
Merged

dump_mir cleanups#145421
bors merged 6 commits intorust-lang:masterfrom
nnethercote:dump_mir-cleanups

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Aug 15, 2025 •
edited
Loading

I found this code hard to read, so I cleaned it up. Details in individual commits.

r? @davidtwco

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 15, 2025
@nnethercote nnethercote marked this pull request as ready for review August 15, 2025 05:46
@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 Aug 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

bjorn3
bjorn3 reviewed Aug 15, 2025
View reviewed changes
@nnethercote
Copy link
Contributor Author

nnethercote commented Aug 27, 2025

It's been almost two weeks, let's try a different reviewer.

r? @davidtwco

@rustbot rustbot assigned davidtwco and unassigned lqd Aug 27, 2025
@rust-lang rust-lang deleted a comment from rustbot Aug 27, 2025
@davidtwco
Copy link
Member

davidtwco commented Aug 29, 2025

@bors r+ rollup

Apologies for the delay in getting to this

@bors
Copy link
Collaborator

bors commented Aug 29, 2025

📌 Commit ac16ad5 has been approved by davidtwco

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 Aug 29, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 29, 2025
@matthiaskrgr
Rollup merge of rust-lang#145421 - nnethercote:dump_mir-cleanups, r=d…
5b232a9 
…avidtwco

`dump_mir` cleanups

I found this code hard to read, so I cleaned it up. Details in individual commits.

r? `@davidtwco`
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 29, 2025
Closed
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 29, 2025
@tgross35
Rollup merge of rust-lang#145421 - nnethercote:dump_mir-cleanups, r=d…
b0af5d2 
…avidtwco

`dump_mir` cleanups

I found this code hard to read, so I cleaned it up. Details in individual commits.

r? ``@davidtwco``
@tgross35 tgross35 mentioned this pull request Aug 29, 2025
Closed
@tgross35
Copy link
Contributor

tgross35 commented Aug 29, 2025

@bors r-

Seems to have bitrotted against 7b13a50, #146020 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 29, 2025
@nnethercote
Inline and remove dump_matched_mir_node.
7ea882d 
It has a single call site.
@nnethercote
Inline and remove dump_mir_for_pass.
3a0d0be 
The code is more readable without it.
@nnethercote
Avoid unnecessary mut-ness for various closures.
d7faa56
@nnethercote
Use trait object references for closures.
d3e2c93 
The dynamic dispatch cost doesn't matter for MIR dumping, which is
perf-insensitive. And it's necessary for the next commit, which will
store some `extra_data` closures in a struct.
@nnethercote
Indent some functions.
2d21c13 
This commit exists purely to simplify reviewing: these functions will
become methods in the next commit. This commit indents them so that the
next commit is more readable.
@nnethercote
Introduce MirDumper and MirWriter.
5ce3797 
MIR dumping is a mess. There are lots of functions and entry points,
e.g. `dump_mir`, `dump_mir_with_options`, `dump_polonius_mir`,
`dump_mir_to_writer`. Also, it's crucial that `create_dump_file` is
never called without `dump_enabled` first being checked, but there is no
mechanism for ensuring this and it's hard to tell if it is satisfied on
all paths. (`dump_enabled` is checked twice on some paths, however!)

This commit introduces `MirWriter`, which controls the MIR writing, and
encapsulates the `extra_data` closure and `options`. Two existing
functions are now methods of this type. It sets reasonable defaults,
allowing the removal of many `|_, _| Ok(())` closures.

The commit also introduces `MirDumper`, which is layered on top of
`MirWriter`, and which manages the creation of the dump files,
encapsulating pass names, disambiguators, etc. Four existing functions
are now methods of this type.
- `MirDumper::new` will only succeed if dumps are enabled, and will
  return `None` otherwise, which makes it impossible to dump when you
  shouldn't.
- It also sets reasonable defaults for various things like
  disambiguators, which means you no longer need to specify them in many
  cases. When they do need to be specified, it's now done via setter
  methods.
- It avoids some repetition. E.g. `dump_nll_mir` previously specifed the
  pass name `"nll"` four times and the disambiguator `&0` three times;
  now it specifies them just once, to put them in the `MirDumper`.
- For Polonius, the `extra_data` closure can now be specified earlier,
  which avoids having to pass some arguments through some functions.
@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@nnethercote
Copy link
Contributor Author

nnethercote commented Aug 31, 2025

Seems to have bitrotted

Fixed.

@bors r=davidtwco

@bors
Copy link
Collaborator

bors commented Aug 31, 2025

📌 Commit 5ce3797 has been approved by davidtwco

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 1, 2025
@Zalathar
Rollup merge of rust-lang#145421 - nnethercote:dump_mir-cleanups, r=d…
991af9d 
…avidtwco

`dump_mir` cleanups

I found this code hard to read, so I cleaned it up. Details in individual commits.

r? `@davidtwco`
@Zalathar Zalathar mentioned this pull request Sep 1, 2025
Merged
bors added a commit that referenced this pull request Sep 1, 2025
@bors
Auto merge of #146072 - Zalathar:rollup-0svnrfe, r=Zalathar
be4e9b7 
Rollup of 6 pull requests

Successful merges:

 - #145421 (`dump_mir` cleanups)
 - #145968 (Add `Bound::copied`)
 - #146004 (resolve: Refactor `struct ExternPreludeEntry`)
 - #146042 (Detect negative literal inferred to unsigned integer)
 - #146046 (Suggest method name with maybe ty mismatch)
 - #146051 (Change std f32 test to pass under Miri)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d17b3fb into rust-lang:master Sep 1, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 1, 2025
rust-timer added a commit that referenced this pull request Sep 1, 2025
@rust-timer
Unrolled build for #145421
df2690b 
Rollup merge of #145421 - nnethercote:dump_mir-cleanups, r=davidtwco

`dump_mir` cleanups

I found this code hard to read, so I cleaned it up. Details in individual commits.

r? ``@davidtwco``
@nnethercote nnethercote deleted the dump_mir-cleanups branch September 1, 2025 07:56
@github-actions github-actions bot mentioned this pull request Sep 5, 2025
Merged
Kobzol pushed a commit to Kobzol/rustc_codegen_cranelift that referenced this pull request Dec 29, 2025
@bors
Auto merge of #146072 - Zalathar:rollup-0svnrfe, r=Zalathar
16c92d4 
Rollup of 6 pull requests

Successful merges:

 - rust-lang/rust#145421 (`dump_mir` cleanups)
 - rust-lang/rust#145968 (Add `Bound::copied`)
 - rust-lang/rust#146004 (resolve: Refactor `struct ExternPreludeEntry`)
 - rust-lang/rust#146042 (Detect negative literal inferred to unsigned integer)
 - rust-lang/rust#146046 (Suggest method name with maybe ty mismatch)
 - rust-lang/rust#146051 (Change std f32 test to pass under Miri)

r? `@ghost`
`@rustbot` modify labels: rollup
christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request Jan 27, 2026
@bors
Auto merge of #146072 - Zalathar:rollup-0svnrfe, r=Zalathar
2023336 
Rollup of 6 pull requests

Successful merges:

 - rust-lang/rust#145421 (`dump_mir` cleanups)
 - rust-lang/rust#145968 (Add `Bound::copied`)
 - rust-lang/rust#146004 (resolve: Refactor `struct ExternPreludeEntry`)
 - rust-lang/rust#146042 (Detect negative literal inferred to unsigned integer)
 - rust-lang/rust#146046 (Suggest method name with maybe ty mismatch)
 - rust-lang/rust#146051 (Change std f32 test to pass under Miri)

r? `@ghost`
`@rustbot` modify labels: rollup
christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request Jan 27, 2026
@bors
Auto merge of #146072 - Zalathar:rollup-0svnrfe, r=Zalathar
65d2d70 
Rollup of 6 pull requests

Successful merges:

 - rust-lang/rust#145421 (`dump_mir` cleanups)
 - rust-lang/rust#145968 (Add `Bound::copied`)
 - rust-lang/rust#146004 (resolve: Refactor `struct ExternPreludeEntry`)
 - rust-lang/rust#146042 (Detect negative literal inferred to unsigned integer)
 - rust-lang/rust#146046 (Suggest method name with maybe ty mismatch)
 - rust-lang/rust#146051 (Change std f32 test to pass under Miri)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

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.91.0

Development

Successfully merging this pull request may close these issues.

7 participants

@nnethercote @rustbot @davidtwco @bors @tgross35 @bjorn3 @lqd