Clarify MIR dialects and phases #137204

nnethercote · 2025-02-18T04:22:04Z

I found the existing code and docs hard to understand.

r? @Zalathar

rustbot · 2025-02-18T04:22:13Z

This PR changes MIR

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

nnethercote · 2025-02-18T04:22:14Z

Best reviewed one commit at a time.

Zalathar · 2025-02-18T07:15:11Z

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

nnethercote · 2025-02-18T08:48:04Z

r? @oli-obk for a second opinion

rustbot · 2025-02-18T08:48:08Z

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 · 2025-02-18T09:36:11Z

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.

compiler/rustc_middle/src/mir/pretty.rs

compiler/rustc_middle/src/mir/syntax.rs

RalfJung · 2025-02-18T10:10:27Z

r? @RalfJung

davidtwco

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 · 2025-02-18T21:49:31Z

I added another commit tweaking the dialect/phase description.

RalfJung · 2025-02-19T13:58:19Z

compiler/rustc_middle/src/mir/syntax.rs

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


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 · 2025-02-19T13:58:43Z

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

Currently many of them exceed 100 chars, which makes them painful to read on a terminal that is 100 chars wide.

I found the dialect/phase distinction quite confusing when I first read these comments. This commit clarifies things a bit.

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 · 2025-02-20T02:36:06Z

I fixed the nit.

@bors r=RalfJung rollup

bors · 2025-02-20T02:36:09Z

📌 Commit 83a7fb6 has been approved by RalfJung

It is now in the queue for this repository.

…nd-phases, r=RalfJung Clarify MIR dialects and phases I found the existing code and docs hard to understand. r? `@Zalathar`

…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

…nd-phases, r=RalfJung Clarify MIR dialects and phases I found the existing code and docs hard to understand. r? ``@Zalathar``

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

…nd-phases, r=RalfJung Clarify MIR dialects and phases I found the existing code and docs hard to understand. r? ```@Zalathar```

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

…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 · 2025-02-21T09:44:17Z

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

huh?

…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

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 · 2025-02-22T13:29:27Z

huh?

So I think you were indeed already assigned.

…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

rustbot assigned Zalathar Feb 18, 2025

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

oli-obk self-assigned this Feb 18, 2025

rustbot assigned davidtwco and unassigned oli-obk and Zalathar Feb 18, 2025

RalfJung reviewed Feb 18, 2025

View reviewed changes

compiler/rustc_middle/src/mir/pretty.rs Show resolved Hide resolved

compiler/rustc_middle/src/mir/syntax.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/mir/syntax.rs Show resolved Hide resolved

rustbot assigned RalfJung and unassigned davidtwco Feb 18, 2025

davidtwco reviewed Feb 18, 2025

View reviewed changes

davidtwco approved these changes Feb 19, 2025

View reviewed changes

RalfJung reviewed Feb 19, 2025

View reviewed changes

nnethercote added 3 commits February 20, 2025 10:26

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.

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.

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 force-pushed the clarify-MIR-dialects-and-phases branch from 7062010 to 83a7fb6 Compare February 20, 2025 02:35

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 mentioned this pull request Feb 20, 2025

Rollup of 14 pull requests #137339

Closed

workingjubilee mentioned this pull request Feb 20, 2025

Rollup of 12 pull requests #137347

Closed

workingjubilee mentioned this pull request Feb 20, 2025

Rollup of 10 pull requests #137352

Closed

matthiaskrgr mentioned this pull request Feb 21, 2025

Rollup of 8 pull requests #137371

Merged

bors merged commit 15a0403 into rust-lang:master Feb 21, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Feb 21, 2025

nnethercote deleted the clarify-MIR-dialects-and-phases branch May 22, 2025 00:13

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

Clarify MIR dialects and phases #137204

Clarify MIR dialects and phases #137204

Uh oh!

Conversation

nnethercote commented Feb 18, 2025

Uh oh!

rustbot commented Feb 18, 2025

Uh oh!

nnethercote commented Feb 18, 2025

Uh oh!

Zalathar commented Feb 18, 2025

Uh oh!

nnethercote commented Feb 18, 2025

Uh oh!

rustbot commented Feb 18, 2025

Uh oh!

nnethercote commented Feb 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Feb 18, 2025

Uh oh!

davidtwco left a comment

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Feb 18, 2025

Uh oh!

RalfJung Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Feb 19, 2025

Uh oh!

nnethercote commented Feb 20, 2025

Uh oh!

bors commented Feb 20, 2025

Uh oh!

oli-obk commented Feb 21, 2025

Uh oh!

Uh oh!

RalfJung commented Feb 22, 2025

Uh oh!

Uh oh!