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

MIR drive-by cleanups #111501

Merged
merged 4 commits into from
May 22, 2023
Merged

MIR drive-by cleanups #111501

merged 4 commits into from
May 22, 2023

Conversation

WaffleLapkin
Copy link
Member

Some random drive-by cleanups I did while working with MIR/THIR.

@rustbot
Copy link
Collaborator

rustbot commented May 12, 2023

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

@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 May 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2023

This PR changes MIR

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned lcnr May 12, 2023
JakobDegen
JakobDegen reviewed May 12, 2023
View reviewed changes
compiler/rustc_middle/src/mir/syntax.rs
@@ -749,6 +749,29 @@ pub enum TerminatorKind<'tcx> {
},
}

impl TerminatorKind<'_> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this in a different file? This one's mostly for type definitions

Copy link
Member Author

Choose a reason for hiding this comment

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

There are similar functions in this file though, like MirPhase::name...

Not sure where to best put this

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, ok. We should probably move those out too, don't worry about it for this PR though

Copy link
Contributor

Choose a reason for hiding this comment

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

The mir::terminator file looks like a proper place.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the drivebycleanupuwu branch from ed5f551 to e874ba8 Compare May 12, 2023 11:06
@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2023

r=me with CI happy

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the drivebycleanupuwu branch from e874ba8 to 693d36f Compare May 15, 2023 12:01
@rustbot
Copy link
Collaborator

rustbot commented May 15, 2023

The Miri subtree was changed

cc @rust-lang/miri

@WaffleLapkin
Copy link
Member Author

So the removal of ! -> ! coercion changes the place where ! is used, changing some miri diagnostics (namely "reached unreachable code"). I think it's fine, but just in case cc @RalfJung. I've blessed this test: 693d36f#diff-483e7d9f8800c2c4dab29c8c5504034bce3e57ae01bc087aefc181c0e93f457a

@rust-log-analyzer

This comment has been minimized.

Sign in to view
oli-obk
oli-obk reviewed May 15, 2023
View reviewed changes
src/tools/miri/tests/fail/never_say_never.rs
Comment on lines 7 to 11
fn main() {
let y = &5;
let x: ! = unsafe {
*(y as *const _ as *const !) //~ ERROR: entering unreachable code
};
f(x)
let x: ! = unsafe { *(y as *const _ as *const !) };
f(x) //~ ERROR: entering unreachable code
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is surprising. How does the MIR change here? The previous MIR didn't even contain an f call, even with -Zmir-opt-level=0

Copy link
Member Author

Choose a reason for hiding this comment

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

The change is essentially

  1. f now has return, not unreachable
  2. main now has additional _3: !
  3. main now has _3 = f(const ZeroSized: !);, not unreachable;

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it looks like the difference is already present after building MIR:

--- ./nightly_mir_dump/t.f.001-000.built.after.mir      2023-05-15 14:25:01.537195340 +0000
+++ ./b_mir_dump/t.f.001-000.built.after.mir    2023-05-15 14:25:07.469143513 +0000
@@ -3,23 +3,9 @@
 fn f(_1: !) -> ! {
     debug x => _1;                       // in scope 0 at ./t.rs:9:6: 9:7
     let mut _0: !;                       // return place in scope 0 at ./t.rs:9:15: 9:16
-    let mut _2: !;                       // in scope 0 at ./t.rs:9:17: 11:2
-    let mut _3: !;                       // in scope 0 at ./t.rs:10:5: 10:6
 
     bb0: {
-        StorageLive(_2);                 // scope 0 at ./t.rs:9:17: 11:2
-        StorageLive(_3);                 // scope 0 at ./t.rs:10:5: 10:6
-        _3 = _1;                         // scope 0 at ./t.rs:10:5: 10:6
-        unreachable;                     // scope 0 at ./t.rs:10:5: 10:6
-    }
-
-    bb1: {
-        StorageDead(_3);                 // scope 0 at ./t.rs:10:5: 10:6
-        unreachable;                     // scope 0 at ./t.rs:9:17: 11:2
-    }
-
-    bb2: {
-        StorageDead(_2);                 // scope 0 at ./t.rs:11:1: 11:2
+        _0 = _1;                         // scope 0 at ./t.rs:10:5: 10:6
         return;                          // scope 0 at ./t.rs:11:2: 11:2
     }
 }

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why NeverToAny should get a special case for ! -> !, but I also don't mind the change in diagnostics here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is "existence of identity coercions makes my other work harder" 😄

Copy link
Member

@RalfJung RalfJung May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's somewhat surprising but well, I am not familiar with those parts of the compiler at all.^^ If this is an invariant we want to enforce, should it be checked somewhere so that we notice when identity coercions come back?

Still, @rust-lang/types should probably take a look.

@WaffleLapkin WaffleLapkin 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 May 17, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the drivebycleanupuwu branch from 693d36f to c24e197 Compare May 17, 2023 10:27
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the drivebycleanupuwu branch from c24e197 to b15f6d7 Compare May 17, 2023 11:51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@WaffleLapkin
Copy link
Member Author

@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 May 22, 2023
@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2023

📌 Commit 140cdcb has been approved by oli-obk

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 May 22, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request May 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2023
@bors
Auto merge of rust-lang#111848 - Dylan-DPC:rollup-7jqydzg, r=Dylan-DPC
8b4b208 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#111501 (MIR drive-by cleanups)
 - rust-lang#111609 (Mark internal functions and traits unsafe to reflect preconditions)
 - rust-lang#111612 (Give better error when collecting into `&[T]`)
 - rust-lang#111756 (Rename `{drop,forget}_{copy,ref}` lints to more consistent naming)
 - rust-lang#111843 (move lcnr to only review types stuff)
 - rust-lang#111844 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit df86200 into rust-lang:master May 22, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakobDegen JakobDegen JakobDegen left review comments

@cjgillot cjgillot cjgillot left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

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

Successfully merging this pull request may close these issues.
9 participants
@WaffleLapkin @rustbot @oli-obk @rust-log-analyzer @bors @RalfJung @cjgillot @JakobDegen @lcnr