[mir-opt] Run SimplifyLocals to a fixedpoint and handle most rvalues #70755
Conversation
wesleywiser
force-pushed
the
simplify_locals_2_electric_boogaloo
branch
from
461ad1c to
cefaf31
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit cefaf31138badbd10cf3d694825d73554d85f258 with merge 20c1dc4b3d1fa2dbc25cc761763c903d4e05fe43... |
| + StorageDead(_6); // bb2[2]: scope 0 at $DIR/simplify-locals-fixedpoint.rs:8:5: 8:6 | ||
| + goto -> bb3; // bb2[3]: scope 0 at $DIR/simplify-locals-fixedpoint.rs:4:5: 8:6 |
There was a problem hiding this comment.
@oli-obk We really gotta stop printing the statement numbers if a test doesn't have NLL annotations which need them.
(speaking of which, are there NLL tests that are not in src/test/mir-opt, but still test MIR somehow?)
| StorageDead(_1); // bb4[0]: scope 0 at $DIR/simplify-locals-fixedpoint.rs:9:1: 9:2 | ||
| return; // bb4[1]: scope 0 at $DIR/simplify-locals-fixedpoint.rs:9:2: 9:2 |
There was a problem hiding this comment.
Hmm, just staring at this: could we normalize $DIR/test-name.rs to $FILE?
It would cause a big src/test/ui diff overall but it might be worth it.
|
@wesleywiser I think there's some confusion going on. You shouldn't need to remap variables in a loop, so I suspect the remapping visitor also does something that should be a separate visitor. You should re-run the marking and the |
|
☀️ Try build successful - checks-azure |
|
Queued 20c1dc4b3d1fa2dbc25cc761763c903d4e05fe43 with parent 74bd074, future comparison URL. |
|
Finished benchmarking try commit 20c1dc4b3d1fa2dbc25cc761763c903d4e05fe43, comparison URL. |
|
@wesleywiser Just realized that if you could the number of "uses" (or "mentions", basically a refcount) of a local you don't even need to redo most of the work, you just remove the uses a statement was providing when you Might even be possible to track the Are we reinventing some other pass here? Even if we are, this might be better. |
| } | ||
| let can_skip = match rvalue { | ||
| Rvalue::Use(op) if can_skip_operand(op) => true, | ||
| Rvalue::Discriminant(_) => true, |
There was a problem hiding this comment.
Should this use !place.is_indirect()?
There was a problem hiding this comment.
I think eliminating a read of an indirect place's discriminant is ok in the same way removing a read of its address is. Is there a situation you're thinking of where that wouldn't be true?
| @@ -0,0 +1,15 @@ | |||
| // compile-flags: -Zmir-opt-level=1 | |||
There was a problem hiding this comment.
It is for regular rustc invocations but for mir-opt tests, -Zmir-opt-level=2 is the default.
wesleywiser
force-pushed
the
simplify_locals_2_electric_boogaloo
branch
3 times, most recently
from
b78a452 to
5614a95
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 5614a9593b5c48bfec465194f1111fa3b0a50fa3 with merge 7b53333c513bab2e3ecb26802629bcca3d21fc9f... |
|
☀️ Try build successful - checks-azure |
|
Queued 7b53333c513bab2e3ecb26802629bcca3d21fc9f with parent d28a464, future comparison URL. |
wesleywiser
force-pushed
the
simplify_locals_2_electric_boogaloo
branch
from
16f53a4 to
feea95a
Compare
This comment has been minimized.
This comment has been minimized.
wesleywiser
force-pushed
the
simplify_locals_2_electric_boogaloo
branch
from
feea95a to
c6747b3
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit c6747b327b899177f80afea60c59756155171b9c with merge 9a9cd24b22d0bf53056b7852d4e43323eab1940e... |
|
☀️ Try build successful - checks-azure |
|
Queued 9a9cd24b22d0bf53056b7852d4e43323eab1940e with parent d12f030, future comparison URL. |
Fixes perf regression in `optimized_mir` query
wesleywiser
force-pushed
the
simplify_locals_2_electric_boogaloo
branch
from
c6747b3 to
da8f3bb
Compare
wesleywiser
changed the title
|
r? @oli-obk |
- Remove reads of indirect `Place`s - Add comments explaining what the algorithm does
|
@bors r+ |
|
📌 Commit 9666d31 has been approved by |
bors
added
the
S-waiting-on-bors
|
@bors rollup=never This is perf impacting. |
|
☀️ Test successful - checks-azure |
|
Post-merged perf results are interesting, the effect on EDIT: |
|
Most of the changes seem to be in LLVM time and based on the "run count" column, it seems like more cgu's are getting regenerated in the incremental scenario? Since this will cause more statements to be removed from the MIR, maybe this is causing code to be partitioned differently? https://github.com/rust-lang/rust/blob/master/src/librustc_middle/mir/mono.rs#L292-L294 |
Emit basic block info for stmts and terminators in MIR dumps only with -Zverbose r? @eddyb as per rust-lang#70755 (comment)
Follow up to review feedback left on #70595.