move Deaggregate pass to post_borrowck_cleanup #71946
Conversation
rust-highfive
added
the
S-waiting-on-review
|
I guess it is worth checking if this has any perf implications. |
|
Awaiting bors try build completion |
|
⌛ Trying commit 7572d47f61795461aeb9aacbace6152a666ae509 with merge 89530668eb34ae5856fd9aabf52a7c8afaa08808... |
src/test/mir-opt/const_prop/aggregate/rustc.main.ConstProp.diff
Outdated
Show resolved
Hide resolved
|
Curiously, this causes incremental test suite failures -- I am not sure what that is about, and have no idea how to debug it. |
|
Hehe, I think that is because the incremental tests expect the changes done to not have an effect on MIR, but the "no effect" only existed because const prop eliminated all changes between the two variants. This is not true anymore due to the const prop regression. |
|
I recall removing annotations from the incremental tests indicating that some changes should have an effect on the optimized MIR when we switched const prop on by default. So that sounds correct to me. |
This comment has been minimized.
This comment has been minimized.
|
That's good, it means we have just one side-effect of this change and not two. :D So is this ConstProp effect something that we should fix before landing this PR? I'm afraid I won't have time to look into that, but I am happy to let this PR sit until someone gets around to that (or else turn it into an issue if we think that might take longer). |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 89530668eb34ae5856fd9aabf52a7c8afaa08808 with parent 43271a3, future comparison URL. |
I think we should fix it. This PR breaks all or at least most of our aggregate + const prop tests. Since we now have in-block const prop even for variables, we can actually write tests for this situation without your PR, so I'm going to do this change independently from your PR, get that merged, and then we merge your PR (whose diff should become much simpler after that). |
|
@oli-obk all right, sounds good! |
RalfJung
added
the
S-blocked
|
🎉 #71953 |
|
Finished benchmarking try commit 89530668eb34ae5856fd9aabf52a7c8afaa08808, comparison URL. |
|
Wow that's a serious perf regression for servo-opt, let's hope it gets fixed by your patch @oli-obk. |
|
:( I'm just getting bad gateway errors when trying to open the perf result |
|
It's working now and ouch! |
|
Looks like CGU problems again https://perf.rust-lang.org/detailed-query.html?commit=89530668eb34ae5856fd9aabf52a7c8afaa08808&base_commit=43271a39adc26d29350d4830b594c6435472815e&benchmark=script-servo-opt&run_name=patched%20incremental:%20println rust-lang/compiler-team#281 is really getting more and more important |
|
I have no idea how to read that "detailed query" page, is there a guide for that somewhere? Somehow there's a "totals" row that sums up to either 136.02% of the time or 56.7% of something without a column header, where I'd expect a total to be 100% of whatever it is, so I am already lost in that row. |
|
Oh wait maybe that weird |
|
Yes, that's intended to be a delta -- I'm forgetting now why it's not just written out... The total time header (e.g. 136.02% in this case) is the amount of time self profile claims it took divided by the amount of time that the cpu clock statistic gave us -- basically wall clock but accounting for parallelism. It can mostly be ignored, just gives you a rough sense how much time we're likely to be "off" by, though I can't tell you why we're sometimes off by so much. |
|
Okay. But why are there three Delta columns that are very different? Is that delta of time, delta of"full executions" (are there "partial executions"?), and "loading delta" (whatever that is)? The first delta could also be delta of "percent of time" as that's the column to its left, not sure. |
|
Let's move this to a separate issue: rust-lang/rustc-perf#648 |
Adding this to my examples list. |
| @@ -74,7 +74,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
| Ok(true) | |||
| } | |||
|
|
|||
| fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> { | |||
| crate fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> { | |||
There was a problem hiding this comment.
Hm... of it's (semi-)public, maybe it should have a doc comment?
Also for consistency, terminator should probably get the same treatment.
|
ok... this is causing stage 2 failures now... Idk how that change could be misoptimizing anything and I won't have time to dig into it until some time next week. |
|
Maybe it is indeed better to make that a separate PR, then. (Quite amazing how hard it can be to reorder these passes.^^) |
RalfJung
added
S-blocked
|
The change would likely not be misoptimizing anything without moving the deaggregation pass as early as you did |
|
but we can move the independent things out of this PR, sure |
|
Whatever you prefer. If you want you can also just make a PR yourself with this branch, at this point you did much more of the work than I did and this is way out of my league anyway.^^ |
oli-obk
added
the
A-MIR
|
If you don't mind, I'd be happy to pull the "remove const prop interning" change into a separate PR. I suspect we can close a number of ICE issues with that change. |
|
That would be great! Thanks |
|
☔ The latest upstream changes (presumably #73081) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Does #71911 (comment) mean we should stop attempting this reordering? |
|
I opened #73203 I don't think we should stop this PR, the discussion is a separate one that we'll be able to resolve in either direction even with this change. |
|
Status: blocked on #73130. |
…ndirects, r=oli-obk Remove const prop for indirects This was only used by one mir-opt test and since it causes buggy behavior under `-Zmir-opt-level=2`, it seems like we should remove it. This was split out from rust-lang#71946. Closes rust-lang#72679 Closes rust-lang#72372 Closes rust-lang#72285
|
I can do the rebase. Since I still can't modify this PR I'll do what you suggested earlier
|
|
Yeah I'm happy for you to take over. :) |
…leywiser move Deaggregate pass to post_borrowck_cleanup Reopen of rust-lang#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 rust-lang#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
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
Deaggregatorpass, 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, likely because deaggregation now runs before const-prop instead of after. I am not sure if that is acceptable, desirable, or a problem.
r? @oli-obk @wesleywiser @eddyb @rust-lang/wg-mir-opt