-
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
Const-prop bugfix: only add propagation inside own block for user variables #71518
Const-prop bugfix: only add propagation inside own block for user variables #71518
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ecstatic-morse (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. |
AHH I forgot to ask for a specific reviewer. Sorry @ecstatic-morse |
r? @wesleywiser |
CC @oli-obk |
Thanks! I'm just waiting for CI to turn green before starting the perf run. |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 8facb48b9efb759cb4055ac3205a400ba5f4142e with merge f498ccd84e2c0f16e72b8bae4683efbbffdc568f... |
☀️ Try build successful - checks-azure |
Queued f498ccd84e2c0f16e72b8bae4683efbbffdc568f with parent 0612568, future comparison URL. |
Is it normal that the timer takes this long? Yesterday it took it 2.5 hours between the "Queued f498ccd with parent 0612568, future comparison URL" comment and the "Finished benchmarking try commit" comment. Today it's been a little bit over 5.5 hours already 🤔 |
It appears to be advancing, though! https://perf.rust-lang.org/status.html It was on |
Finished benchmarking try commit f498ccd84e2c0f16e72b8bae4683efbbffdc568f, comparison URL. |
OH BOY this is a bad one. The regressions in performance seem to be here. They lie mostly on memory performance (so I have a couple theories. My main one is that the clone of My other theories are:
|
Removed the |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 390f416e12c76e08a6cff8a4e46432fa06e8e04d with merge 845e3f5d8ad7afbe9b98ff3588f44ddceaf379bf... |
☀️ Try build successful - checks-azure |
Queued 845e3f5d8ad7afbe9b98ff3588f44ddceaf379bf with parent 40008dc, future comparison URL. |
Sorry, I probably should have explained a bit more in the original PR. Generally, we just look at the instructions metric because the others have high variance even on the most innocuous of changes. The perf results here look ok to me. I suspect it's the combination of this PR with the other PR that is causing the issue. |
Dangit, look at that regression! What happened? Btw here it is when compared to the same No but seriously, what happened? I'm guessing that either the |
Cont: yep, |
I was just testing a new approach that does not call The advantage of this is that after visiting a The disadvantage is that, apart from a check in This approach of gating with * Small digression: that assert checks whether or not the two |
Rebased to fit in with the latest changes to MIR output |
This is mostly ready, I think. I still want to add a medium-sized test that showcases how far const-prop has gone, and what things it still lacks. But that can be done on a different PR. The goal of that test is mostly documentation. Anything that might be missing? |
src/test/mir-opt/const_prop/usize_literal_propagation/32bit/rustc.main.ConstProp.diff
Outdated
Show resolved
Hide resolved
src/test/mir-opt/const_prop/usize_literal_propagation/64bit/rustc.main.ConstProp.diff
Outdated
Show resolved
Hide resolved
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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. Of course, the OOB test has to fail because it uses indices, which are |
…ed some doc comments in `mir/transform/const_prop.rs`
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.
Wonderful! Thanks so much for working on this 👍
@bors r+ rollup=never |
📌 Commit 16ebaf9 has been approved by |
@bors p=1 |
☀️ Test successful - checks-azure |
update test output
rustup rust-lang/rust#71518 update test output changelog: none
Rustup to rust-lang/rust#71518 changelog: none
⛵️ |
A testing spinoff of #71298. This one only adds the const-prop for locals that are user variables.