Split dead store elimination off dest prop #97158
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
tmiasko
reviewed
May 19, 2022
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit e54b7e0a6573ca8eb8a308363449835b27647cbd with merge edc0389da8df533107b33d6dba3ef7021209ede0... |
|
☀️ Try build successful - checks-actions |
|
Queued edc0389da8df533107b33d6dba3ef7021209ede0 with parent 67a9bcb, future comparison URL. |
|
Finished benchmarking commit (edc0389da8df533107b33d6dba3ef7021209ede0): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
rustbot
added
perf-regression
tmiasko
added
S-waiting-on-author
tmiasko
reviewed
May 20, 2022
memoryruins
reviewed
May 20, 2022
|
The update should address all the outstanding review comments. I've also added in a check that ensure we don't optimize out ptr2int casts. Once CI passes, I'll restart perf as well |
|
@bors try @rust-timer queue @rustbot ready Oh also, I still need to add some actual tests. Kind of forgot to do that because of all the existing MIR opt tests this affects |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
S-waiting-on-review
|
⌛ Trying commit e17fcacd7035e80d6ceab06aee48da58a675c9e1 with merge ee288a61fe1e3c195eeadd3527ccf09eddf63506... |
|
☀️ Try build successful - checks-actions |
|
Queued ee288a61fe1e3c195eeadd3527ccf09eddf63506 with parent e6a4afc, future comparison URL. |
|
Interesting. The test failed because bors turns off optimizations for tests. I've asked T-Infra on Zulip, also cc @cjgillot who answered questions about these tests last time I had one and may want to weigh in |
|
The simplest way would be to force either optimization or no-optimization on those tests. |
|
For all incremental tests or just for those? |
|
All those in the |
|
@cjgillot done. There were two files ( |
|
@bors r=tmiasko,cjgillot |
|
📌 Commit 7a99da1 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
May 28, 2022
Tested on commit rust-lang/rust@6831417. Direct link to PR: <rust-lang/rust#97158> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
This was referenced May 28, 2022
|
Finished benchmarking commit (6831417): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Footnotes |
Merged
|
For perf triage: In non-opt builds, things generally improved which means we are optimizing quite some things that LLVM doesn't catch. The only regressions are in bitmaps and some wg-grammar stuff. I checked with cachegrind, it points to For opt builds, some things slowed down. This is to be expected to some extent, since rustc is optimizing things it wasn't before and so doing more work. I ran cachegrind on a couple of the slower benchmarks and nothing stood out to me. The serde full one is reproduced below if anyone else wants to take a look. Likely the only way to improve upon the regressions is to make this pass faster. That is certainly desirable, but I don't know of any low hanging fruit there. Serde Full CachegrindIn any case, this 1) is a performance improvement in most cases, 2) is a de-facto prerequisite for dest prop, and 3) causes the compiler to emit faster object code, so I generally think the regressions are justified. Meta sidenote: What are the rules around applying the "perf-regression-triaged" label? If I am investigating compiler perf anyway, should I also be applying the label or is this something that should be reserved for wg-perf/people with r+ rights? |
@rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This splits off a part of #96451 . I've added this in as its own pass for now, so that it actually runs, can be tested, etc. In the dest prop PR, I'll stop invoking this as its own pass, so that it doesn't get invoked twice.
r? @tmiasko