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.

Sign up for GitHub

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

Jump to bottom

Remove unused set-discriminant statements and assignments regardless of rvalue #77876

Merged
merged 6 commits into from
Oct 27, 2020
Merged

Remove unused set-discriminant statements and assignments regardless of rvalue #77876

merged 6 commits into from
Oct 27, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 12, 2020

  • Represent use counts with u32
  • Unify use count visitors
  • Change RemoveStatements visitor into a function
  • Remove unused set-discriminant statements
  • Use exhaustive match to clarify what is being optimized
  • Remove unused assignments regardless of rvalue kind

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 12, 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 12, 2020

⌛ Trying commit 443181e9a31cf4eb006b786261b932c978940155 with merge 2fc7421b922ca95716a1e8b2863897f8f661d0e0...

@bors
Copy link
Contributor

bors commented Oct 12, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 2fc7421b922ca95716a1e8b2863897f8f661d0e0 (2fc7421b922ca95716a1e8b2863897f8f661d0e0)

@rust-timer
Copy link
Collaborator

Queued 2fc7421b922ca95716a1e8b2863897f8f661d0e0 with parent 8dae8cd, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2fc7421b922ca95716a1e8b2863897f8f661d0e0): 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 tmiasko force-pushed the simplify-locals branch 2 times, most recently from 0a63cff to 7f17305 Compare October 17, 2020 17:53
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 22, 2020

r? @wesleywiser

@rust-highfive rust-highfive assigned wesleywiser and unassigned varkor Oct 22, 2020
wesleywiser
wesleywiser approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great and I'm glad to see tests covering the new behavior!

@wesleywiser wesleywiser added the A-mir-opt Area: MIR optimizations label Oct 26, 2020
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2020

📌 Commit 89314a4e92e4e675c10ea6db9ca3914af5a81f26 has been approved by wesleywiser

@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 Oct 26, 2020
@bors
Copy link
Contributor

bors commented Oct 26, 2020

⌛ Testing commit 89314a4e92e4e675c10ea6db9ca3914af5a81f26 with merge 3cff5cc956b22a8c0f810cd9009c29c0ece46e9c...

@bors
Copy link
Contributor

bors commented Oct 26, 2020

💔 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 Oct 26, 2020
@tmiasko
simplify-locals: Represent use counts with u32
1126953
@tmiasko
simplify-locals: Unify use count visitors
a6b64be 
The simplify locals implementation uses two different visitors to update
the locals use counts. The DeclMarker calculates the initial use counts.
The StatementDeclMarker updates the use counts as statements are being
removed from the block.

Replace them with a single visitor that can operate in either mode,
ensuring consistency of behaviour.

Additionally use exhaustive match to clarify what is being optimized.

No functional changes intended.
@tmiasko
simplify-locals: Change RemoveStatements visitor into a function
e1e48ae 
No functionl changes intended.
@tmiasko
simplify-locals: Remove unused set-discriminant statements
52d3782 
Update affected ui & incremental tests to use a user declared variable
bindings instead of temporaries. The former are preserved because of
debuginfo, the latter are not.
@tmiasko
simplify-locals: Remove unused assignments regardless of rvalue kind
4c3e06a
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 26, 2020

Added overflow-checks=off to the test and updated the output.

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2020

📌 Commit 4c3e06a has been approved by wesleywiser

@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 Oct 26, 2020
@bors
Copy link
Contributor

bors commented Oct 26, 2020

⌛ Testing commit 4c3e06a with merge a4d30a7...

@bors
Copy link
Contributor

bors commented Oct 27, 2020

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing a4d30a7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 27, 2020
@bors bors merged commit a4d30a7 into rust-lang:master Oct 27, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 27, 2020
This was referenced Oct 27, 2020
Closed
Closed
@tmiasko tmiasko deleted the simplify-locals branch October 27, 2020 09:12
@Mark-Simulacrum
Copy link
Member

Performance results, as expected, are an improvement on the ctfe stress test. That said, there does appear to be a regression on the cranelift codegen benchmark in LLVM -- perhaps these weren't quite unused? Any thoughts here?

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 27, 2020 •
edited
Loading

Those changes are completely boring from LLVM perspective. They remove dead code and affect size estimates accordingly. They might change the unit partitioning as a side effect.

Testing locally, I do have a different unit partitioning for full builds of cranelift-codegen. I see no reason to consider this to be anything different from codegen partitioning noise.

@Mark-Simulacrum
Copy link
Member

Ah, ok. Makes sense. Thanks for investigating!

@Alexendoo Alexendoo mentioned this pull request Oct 28, 2020
Closed
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Dec 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
A-mir-opt Area: MIR optimizations 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
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
9 participants
@tmiasko @rust-highfive @jonas-schievink @rust-timer @bors @wesleywiser @Mark-Simulacrum @varkor @rustbot