-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Fixed const propagation not working in a particular scenario #71298
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Some notes on this PR for whoever is reading and wants some explanations about the changes: This was added in order to have I originally made No idea why the Not being able to const-prop over |
Sorry, I'm super rusty with This test changed its output. It seems to me that it's now more performant (basically, it's being optimized better now). Please help me here though, because I'm not 100% sure. I definitely need help here. These MIR changes are way too complicated for my current understanding of MIR >.< Did I cause a regression, or is this test also optimizing better now? |
Missed rustfmt. Brb with correct format |
This comment has been minimized.
This comment has been minimized.
I'm working on a fix for the errors shown in CI. Will be back as soon as my computer finishes running the test suite again! |
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.
Thanks for the PR! I left a few comments. Let me know if you have any questions.
src/test/mir-opt/const_prop/usize_literal_propagation/rustc.main.ConstProp.diff
Outdated
Show resolved
Hide resolved
r? @wesleywiser |
Kay, uploaded the changes that let the old tests pass again (const-prop was now detecting UB at compile time in 3 of them <3) |
What IS that CI failure? Its output is so weird. It somehow expects ints with 8 0's instead of 16 0's. Is this a type inference problem, or is it perhaps 32 vs 64 bit thing? 🤔 |
This comment has been minimized.
This comment has been minimized.
Yeah, it looks like the tests are failing on 32-bit ARM. For the
|
Sorry, I forgot, you'll also need to For 64-bit:
For 32-bit:
(Assuming you're on Linux. You may also need to install some 32-bit compatibility libraries like this mentions.) |
Got it! (I think, that seems fairly straightforward). And yes, I'm on Linux :3 |
This comment has been minimized.
This comment has been minimized.
You might want to rebase when you get a chance so that CI will run and we can see if this improved the failing UI tests (it should have). |
This comment has been minimized.
This comment has been minimized.
(I'm recompiling everthing because the repo got into some weird state and I decided to reset git's memory, in a way, by cloning again at these commits) (therefore, I'm basically offline until I manage to recompile everything :v) |
Kay, re-blessed those two tests. I checked the output manually and whatever they're doing, I think those programs are gonna have the same output as their predecessors. Now on to checking if the rest of the test suite is a-ok |
Oh wow, those failures are weird. What do they even mean? |
Okay, I'll have to generate the MIR for those snippets, or I'll have no idea of what to do. They look very arcane to me (like But hey, on the good side: only 2 tests failed. |
Oh, okay, that's not entirely accurate. Only 2 fail, but on the |
Okay, wait... I don't get it. There are errors for which the test complains that the For example, the first failing test gives this output. And for good measure, these are the contents of the They're all the same? What's going on? 🤔 |
(@wesleywiser?) |
I also am very confused. What may be happening is that there's a miscompilation introduced by these changes which is affecting the test harness itself and causing strange issues like this. |
@felix91gr Would you mind opening another PR with just commits 1, 2 and 3 in it? We can test there if the issue is propagating vars or the function args change. |
Sure! Lemme make a couple of new branches, will be back with two testing PRs :3 |
The two changes are up! What I did to dodge the aliasing of |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 6e586b8 with merge 075d9ec4e3198749f4bb6e6f3bb2b29b8eceb815... |
☀️ Try build successful - checks-azure |
Queued 075d9ec4e3198749f4bb6e6f3bb2b29b8eceb815 with parent 0862458, future comparison URL. |
Finished benchmarking try commit 075d9ec4e3198749f4bb6e6f3bb2b29b8eceb815, comparison URL. |
All of that regression time is spent in LTO, because we go from 16 codegen units to 44! So technically this PR is now an improvement, because it allows rustc to split the crate into more codegen units? Idk... is optimizing for LTO compilation speed actually something we care about? |
Yay!
Wait, I'm not sure I'm understanding you correctly. You mean that:
Probably, but not at this stage. Specially if we are to fully decouple the compiler into libraries in the future. My guess is that we want to optimize LTO compilation speed, but only starting at the codegen stage and not before. Because MIR is at the end of the day going to have to do what MIR's going to have to do, right? Be a place for optimizations that benefit all Rust backends, a place for static analysis and a place for compile-time computation, right? I think we are doing the right thing here :) |
I think what's actually happening is that before, the patched incremental test had 16 codegen units get regenerated and now 44 are being regenerated. I believe the total number of codgen units is directly tied to the number of modules in the crate which for our perf tests, is always the same. So this is a clear regression. I think unfortunately at this time we can't merge this exactly as is because of the large incremental regression and no correspondingly large improvements. However, this PR definitely moves us closer to our end goal of high-quality MIR optimizations, it's just currently hampered by other parts of the compiler. I'm also very sympathetic to the clippy use case the original issue there completed. At this time, we can do this:
Clippy unfortunately won't see the effects of point 2 because it doesn't use unstable optimizations. However, it should be relatively easy to add some logic to clippy that does a very simple version of const-prop and would resolve your issue. After point 1 is merged, I think all you will need to do is look for any locals in a Let me know if you have any questions and I'll be happy to try to answer them 🙂 |
💓
How does that work? I'm guessing clippy can't call upon a Another idea is that maybe this can be gated in Clippy as a nightly-only feature, as well. So that we don't write new code just because we want the feature to appear on stable right now. But if you say it's actually easy, maybe we can give it a try. @oli-obk how straightforward do you think it'll be to implement the lint if we have (1) but not (2) on hand? |
@wesleywiser yes!
|
We're not actually sure yet. We've noticed more performance issues cropping up because of the current algorithm so it's something we need to look into soon. Our next step is to hold a planning meeting with the rest of the compiler team. I'll be sure to ping you when I fill out the meeting issue template so you know when that happens!
I'm not really sure. I suspect there's some improvements to the current algorithm we could find and land in the next few months. Finding a significantly better algorithm will probably take much longer.
Yes, absolutely! All of the compiler team triage and design meetings happen on the Rust Zulip instance. Triage meetings are Thursday mornings at 10am US Eastern time. Design meetings are Friday mornings at 10am US Eastern time. Anyone is welcome to attend either.
There's a few places I'd look:
|
…k_prop, r=wesleywiser Const-prop bugfix: only add propagation inside own block for user variables A testing spinoff of rust-lang#71298. This one only adds the const-prop for locals that are user variables.
Closed since #71697 takes the baton on the conversation and remaining points to implement ✨ |
As shown here, constant propagation is missing at least the following scenario:
It is currently being const propagated like this:
And it should behave like this:
This PR addresses this scenario, and changes the result of two other
mir-opt
tests. I will need help figuring out if those tests cases are being further optimized, or if I ran into a regression.(CC @oli-obk)