Skip to content
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

Remove storage markers if they won't be used during code generation #78360

Merged
merged 3 commits into from
Mar 1, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 25, 2020

The storage markers constitute a substantial portion of all MIR
statements. At the same time, for builds without any optimizations,
the storage markers have no further use during and after MIR
optimization phase.

If storage markers are not necessary for code generation, remove them.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 25, 2020

⌛ Trying commit 638ae965adce2470ddf9937dc0674af9b490622e with merge 2cb5cb2ebd84701a032939c589afbc536d832a40...

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 25, 2020

Thanks for perf-run. At this point it is mostly an experiment.

r? @ghost

@bors
Copy link
Contributor

bors commented Oct 25, 2020

☀️ Try build successful - checks-actions
Build commit: 2cb5cb2ebd84701a032939c589afbc536d832a40 (2cb5cb2ebd84701a032939c589afbc536d832a40)

@rust-timer
Copy link
Collaborator

Queued 2cb5cb2ebd84701a032939c589afbc536d832a40 with parent f392479, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2cb5cb2ebd84701a032939c589afbc536d832a40): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 26, 2020

An improvement for full and incremental builds of larger benchmarks. A minor regression for very small benchmarks.

  • full: style-servo-debug -1.2%, cranelift-codegen-debug -1.0%, cargo-debug -0.4%.
  • incr-unchanged: style-servo-debug -1.4%, cranelift-codegen-debug -2.5%, cargo-debug: -1.5%
  • a big regression in incr-patched for clap-rs-debug +33.9% as a result of an additional codegen unit (size estimates used for partitioning are based on the total number of statements, which are changed by this PR).

@tmiasko tmiasko closed this Oct 26, 2020
@tmiasko tmiasko deleted the storage-markers branch October 26, 2020 12:39
@tmiasko tmiasko restored the storage-markers branch January 12, 2021 21:56
@tmiasko tmiasko reopened this Jan 12, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 12, 2021

This a simple transformation removing storage markers at opt-level=0 where they won't have any further use. The CTFE having its own dedicated MIR will be unaffected.

r? @wesleywiser

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

Should do a perf run for the new code then?

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 13, 2021

A new perf run would be nice to have. Thanks.

@estebank
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 13, 2021

⌛ Trying commit 5bc26c8ece9b697078c730fff254e297afacba6b with merge 07d546cab150e55802417ec8b182be1a5522ad45...

@bors
Copy link
Contributor

bors commented Jan 13, 2021

☀️ Try build successful - checks-actions
Build commit: 07d546cab150e55802417ec8b182be1a5522ad45 (07d546cab150e55802417ec8b182be1a5522ad45)

@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Testing commit 31c01be3f262afb19a28fe79d0889905171f3e9f with merge 01bf843c8bba585821820c45b7605889f914a9e5...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 24, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2021
The storage markers constitute a substantial portion of all MIR
statements. At the same time, for builds without any optimizations,
the storage markers have no further use during and after MIR
optimization phase.

If storage markers are not necessary for code generation, remove them.
The storage markers are removed at -C opt-level=0 and as a result output
of mir opt tests vary based on used optimization level. Use opt-level=1
for mir-opt tests to avoid the issue.
@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 28, 2021

Rebased and added -O flag to a try_identity codegen test that requires storage markers so that MIR optimization applies.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 1, 2021

@bors r=wesleywiser,oli-obk

@bors
Copy link
Contributor

bors commented Mar 1, 2021

📌 Commit 57de468 has been approved by wesleywiser,oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021
@bors
Copy link
Contributor

bors commented Mar 1, 2021

⌛ Testing commit 57de468 with merge 09db057...

@bors
Copy link
Contributor

bors commented Mar 1, 2021

☀️ Test successful - checks-actions
Approved by: wesleywiser,oli-obk
Pushing 09db057 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 1, 2021
@bors bors merged commit 09db057 into rust-lang:master Mar 1, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 1, 2021
@tmiasko tmiasko deleted the storage-markers branch March 1, 2021 18:58
@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2021

Storage markers are also used by Miri, and this caused some really strange test failures where we fail to detect some UB with optimizations enabled. I am not sure what is going on...

FWIW, would be good to ping @rust-lang/miri for MIR optimization changes that could affect UB detection. This is the second time in a week that I had to dig through rustc PR history to figure out why a test suddenly fails.^^

bors added a commit to rust-lang/miri that referenced this pull request Mar 2, 2021
rustup; fix tests for new MIR optimization

Somehow rust-lang/rust#78360 manages to mask UB. This would make sense if there were loops or things like that, but there are not, so really this is just very confusing...
bors added a commit to rust-lang/miri that referenced this pull request Mar 2, 2021
rustup; fix tests for new MIR optimization

Somehow rust-lang/rust#78360 manages to mask UB. This would make sense if there were loops or things like that, but there are not, so really this is just very confusing...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.