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

Clarify MIR dialects and phases #137204

Merged
merged 3 commits into from
Feb 21, 2025
Merged

Clarify MIR dialects and phases #137204

merged 3 commits into from
Feb 21, 2025

Conversation

nnethercote
Copy link
Contributor

I found the existing code and docs hard to understand.

r? @Zalathar

@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 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@oli-obk oli-obk self-assigned this Feb 18, 2025
@Zalathar
Copy link
Contributor

Looks good to me, but I'm happy to wait for a second opinion from someone with more experience in this area.

@nnethercote
Copy link
Contributor Author

r? @oli-obk for a second opinion

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@nnethercote
Copy link
Contributor Author

Could not assign reviewer from: oli-obk. User(s) oli-obk are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

Ok, then, r? @davidtwco for a second opinion.

@rustbot rustbot assigned davidtwco and unassigned oli-obk and Zalathar Feb 18, 2025
@RalfJung
Copy link
Member

r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned davidtwco Feb 18, 2025
davidtwco
davidtwco reviewed Feb 18, 2025
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, I don't have a strong preference re https://github.com/rust-lang/rust/pull/137204/files#r1959437174 but if you want to adopt it then that's good too, r=me.

@nnethercote
Copy link
Contributor Author

I added another commit tweaking the dialect/phase description.

RalfJung
RalfJung reviewed Feb 19, 2025
View reviewed changes
compiler/rustc_middle/src/mir/syntax.rs Outdated
/// differences come in two forms: Dialects and phases.
/// The MIR pipeline is structured into a few major dialects, with one or more phases within each
/// dialect. A MIR flavor is identified by a dialect-phase pair. A single `MirPhase` value
/// specifies such a pair. All flavors of MIR use the same data structure.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// specifies such a pair. All flavors of MIR use the same data structure.
/// specifies such a pair. All flavors of MIR use the same data structure to represent the program.

@RalfJung
Copy link
Member

r=me with a final nit while we are at it.

@nnethercote
Reflow MirPhase comments.
c9fbaab 
Currently many of them exceed 100 chars, which makes them painful to
read on a terminal that is 100 chars wide.
@nnethercote
Improve MIR phase comments.
c039533 
I found the dialect/phase distinction quite confusing when I first read
these comments. This commit clarifies things a bit.
@nnethercote
Improve how the MIR dialect/phase index is reported.
83a7fb6 
The only visible change is to the filenames produce by `-Zdump-mir`.
E.g. before and after:
```
h.main.003-000.analysis-post-cleanup.after.mir
h.main.2-2-000.analysis-post-cleanup.after.mir
```
It also fixes a FIXME comment.
@nnethercote nnethercote force-pushed the clarify-MIR-dialects-and-phases branch from 7062010 to 83a7fb6 Compare February 20, 2025 02:35
@nnethercote
Copy link
Contributor Author

I fixed the nit.

@bors r=RalfJung rollup

@bors
Copy link
Contributor

bors commented Feb 20, 2025

📌 Commit 83a7fb6 has been approved by RalfJung

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, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 20, 2025
@workingjubilee
Rollup merge of rust-lang#137204 - nnethercote:clarify-MIR-dialects-a…
16f0f35 
…nd-phases, r=RalfJung

Clarify MIR dialects and phases

I found the existing code and docs hard to understand.

r? `@Zalathar`
@workingjubilee workingjubilee mentioned this pull request Feb 20, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2025
@bors
Auto merge of rust-lang#137339 - workingjubilee:rollup-esgu55d, r=wor…
2c9b4c5 
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#131651 (Create a generic AVR target: avr-none)
 - rust-lang#136473 (infer linker flavor by linker name if it's sufficiently specific)
 - rust-lang#136608 (Pass through of target features to llvm-bitcode-linker and handling them)
 - rust-lang#136985 (Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited))
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137270 (Fix `*-win7-windows-msvc` target since 26eeac1)
 - rust-lang#137298 (Check signature WF when lowering MIR body)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137312 (Update references to cc_detect.rs)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137318 (Workaround Cranelift not yet properly supporting vectors smaller than 128bit)
 - rust-lang#137322 (Update docs for default features of wasm targets)
 - rust-lang#137324 (Make x86 QNX target name consistent with other Rust targets)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 20, 2025
@workingjubilee
Rollup merge of rust-lang#137204 - nnethercote:clarify-MIR-dialects-a…
566c4d4 
…nd-phases, r=RalfJung

Clarify MIR dialects and phases

I found the existing code and docs hard to understand.

r? ``@Zalathar``
@workingjubilee workingjubilee mentioned this pull request Feb 20, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2025
@bors
Auto merge of rust-lang#137347 - workingjubilee:rollup-8l6sr60, r=<try>
252d70e 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#128080 (Specify scope in `out_of_scope_macro_calls` lint)
 - rust-lang#135354 ([Debuginfo] Add MSVC Synthetic and Summary providers to LLDB)
 - rust-lang#135630 (add more `s390x` target features)
 - rust-lang#136089 (Reduce `Box::default` stack copies in debug mode)
 - rust-lang#136148 (Optionally add type names to `TypeId`s.)
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137302 (Use a probe to avoid registering stray region obligations when re-checking drops in MIR typeck)
 - rust-lang#137305 (Tweaks in and around `rustc_middle`)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137333 (Use `edition = "2024"` in the compiler (redux))

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: test-various
try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
try-job: i686-msvc-1
try-job: i686-msvc-2
try-job: i686-mingw-1
try-job: i686-mingw-2
try-job: i686-mingw-3
try-job: x86_64-gnu-nopt
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 20, 2025
@workingjubilee
Rollup merge of rust-lang#137204 - nnethercote:clarify-MIR-dialects-a…
42f4131 
…nd-phases, r=RalfJung

Clarify MIR dialects and phases

I found the existing code and docs hard to understand.

r? ```@Zalathar```
@workingjubilee workingjubilee mentioned this pull request Feb 20, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
@bors
Auto merge of rust-lang#137352 - workingjubilee:rollup-mwikym3, r=<try>
2aff615 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#128080 (Specify scope in `out_of_scope_macro_calls` lint)
 - rust-lang#135630 (add more `s390x` target features)
 - rust-lang#136089 (Reduce `Box::default` stack copies in debug mode)
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137302 (Use a probe to avoid registering stray region obligations when re-checking drops in MIR typeck)
 - rust-lang#137305 (Tweaks in and around `rustc_middle`)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137333 (Use `edition = "2024"` in the compiler (redux))

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: aarch64-gnu
try-job: armhf-gnu
try-job: i686-mingw-1
try-job: i686-mingw-2
try-job: i686-mingw-3
try-job: test-various
try-job: x86_64-gnu-nopt
try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
@bors
Auto merge of rust-lang#137352 - workingjubilee:rollup-mwikym3, r=wor…
95a4dce 
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#128080 (Specify scope in `out_of_scope_macro_calls` lint)
 - rust-lang#135630 (add more `s390x` target features)
 - rust-lang#136089 (Reduce `Box::default` stack copies in debug mode)
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137302 (Use a probe to avoid registering stray region obligations when re-checking drops in MIR typeck)
 - rust-lang#137305 (Tweaks in and around `rustc_middle`)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137333 (Use `edition = "2024"` in the compiler (redux))

r? `@ghost`
`@rustbot` modify labels: rollup
@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2025

User(s) oli-obk are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

huh?

@matthiaskrgr matthiaskrgr mentioned this pull request Feb 21, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
@bors
Auto merge of rust-lang#137371 - matthiaskrgr:rollup-3qkdqar, r=matth…
71e06b9 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128080 (Specify scope in `out_of_scope_macro_calls` lint)
 - rust-lang#135630 (add more `s390x` target features)
 - rust-lang#136089 (Reduce `Box::default` stack copies in debug mode)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137302 (Use a probe to avoid registering stray region obligations when re-checking drops in MIR typeck)
 - rust-lang#137305 (Tweaks in and around `rustc_middle`)
 - rust-lang#137313 (Some codegen_llvm cleanups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 15a0403 into rust-lang:master Feb 21, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 21, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
@rust-timer
Unrolled build for rust-lang#137204
715ddd3 
Rollup merge of rust-lang#137204 - nnethercote:clarify-MIR-dialects-and-phases, r=RalfJung

Clarify MIR dialects and phases

I found the existing code and docs hard to understand.

r? `@Zalathar`
@RalfJung
Copy link
Member

huh?

image

So I think you were indeed already assigned.

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

@RalfJung RalfJung RalfJung left review comments

@Zalathar Zalathar Zalathar left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@RalfJung RalfJung

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.87.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rustbot @Zalathar @RalfJung @bors @oli-obk @davidtwco