-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
move Deaggregate pass to post_borrowck_cleanup #73656
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
150dab1
to
c350951
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oh so the reference bug also interacts with this? Or is that just to avoid conflicts? |
just to avoid conflicts, the bugfix changes MIR that this PR also changes. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c350951
to
37a4567
Compare
} | ||
|
||
bb3: { | ||
discriminant(_6) = 0; // scope 1 at $DIR/funky_arms.rs:17:18: 17:38 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesleywiser the bug used to be that this (the last assignment to _6
in this MIR) assigned a constant to _6
and didn't erase it at the end of the block.
StorageLive(_18); // scope 2 at $DIR/funky_arms.rs:25:51: 25:54 | ||
_18 = _2; // scope 2 at $DIR/funky_arms.rs:25:51: 25:54 | ||
StorageLive(_19); // scope 2 at $DIR/funky_arms.rs:25:56: 25:60 | ||
_19 = _6; // scope 2 at $DIR/funky_arms.rs:25:56: 25:60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
causing it to be read here, even though both assignments flow into this block
37a4567
to
13529d6
Compare
This comment has been minimized.
This comment has been minimized.
…leywiser Const prop: erase all block-only locals at the end of every block I messed up this erasure in rust-lang#73656 (comment). I think it is too fragile to have the previous scheme. Let's benchmark the new scheme and see what happens. r? @wesleywiser cc @felix91gr
13529d6
to
3b5c65d
Compare
@wesleywiser this is ready for review again |
src/test/mir-opt/funky_arms/rustc.float_to_exponential_common.ConstProp.diff
Outdated
Show resolved
Hide resolved
3b5c65d
to
94bcb4e
Compare
94bcb4e
to
2d2049c
Compare
ping @wesleywiser I did a rebase as discussed on zulip and it still applies cleanly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small question
src/test/mir-opt/generator-tiny/rustc.main-{{closure}}.generator_resume.0.mir
Outdated
Show resolved
Hide resolved
src/test/mir-opt/generator-tiny/rustc.main-{{closure}}.generator_resume.0.mir
Outdated
Show resolved
Hide resolved
src/test/mir-opt/generator-tiny/rustc.main-{{closure}}.generator_resume.0.mir
Outdated
Show resolved
Hide resolved
921c120
to
e15f3e0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ath, r=wesleywiser Check whether locals are too large instead of whether accesses into them are too large Essentially this stops const prop from attempting to optimize ```rust let mut x = [0_u8; 5000]; x[42] = 3; ``` I don't expect this to be a perf improvement without rust-lang#73656 (which is also where the lack of this PR will be a perf regression). r? @wesleywiser
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e15f3e0
to
307d0d8
Compare
@bors r=wesleywiser |
📌 Commit 307d0d8 has been approved by |
☀️ Test successful - checks-actions, checks-azure |
This resulted in small regression to instruction counts on |
Reopen of #71946
Only the second commit is from this PR, the other commit is a bugfix that's in the process of getting merged. I'll rebase once that's done
In #70073 MIR pass handling got reorganized, but with the goal of not changing behavior (except for disabling some optimizations on opt-level = 0). But there we realized that the Deaggregator pass, while conceptually more of a "cleanup" pass (and one that should be run before optimizations), was run in the middle of the optimization chain. Likely this is an accident of history, so I suggest we try and clean that up by making it a proper cleanup pass.
This does change mir-opt output, because deaggregation now runs before const-prop instead of after.
r? @wesleywiser @rust-lang/wg-mir-opt
cc @RalfJung