Skip to content

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

Merged
bors merged 6 commits into
rust-lang:masterfrom
tmiasko:simplify-locals
Oct 27, 2020
Merged

Remove unused set-discriminant statements and assignments regardless of rvalue#77876
bors merged 6 commits into
rust-lang:masterfrom
tmiasko:simplify-locals

Conversation

@tmiasko

@tmiaskotmiasko commented Oct 12, 2020

Copy link
Copy Markdown
Contributor
  • 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

rust-highfive commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

r? @varkor

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

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

jonas-schievink commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Oct 12, 2020

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Oct 13, 2020

Copy link
Copy Markdown
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
tmiaskoforce-pushed the simplify-locals branch 2 times, most recently from 0a63cff to 7f17305CompareOctober 17, 2020 17:53
@tmiasko

tmiasko commented Oct 22, 2020

Copy link
Copy Markdown
ContributorAuthor

r? @wesleywiser

wesleywiser
wesleywiser reviewed Oct 24, 2020
View reviewed changes
Comment threadcompiler/rustc_mir/src/transform/simplify.rs Outdated
wesleywiser
wesleywiser approved these changes Oct 26, 2020
View reviewed changes

@wesleywiserwesleywiser left a comment

Copy link
Copy Markdown
Member

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!

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

wesleywiser commented Oct 26, 2020

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 89314a4e92e4e675c10ea6db9ca3914af5a81f26 has been approved by wesleywiser

@borsbors 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

bors commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@borsbors 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

tmiasko commented Oct 26, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@wesleywiser

wesleywiser commented Oct 26, 2020

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 4c3e06a has been approved by wesleywiser

@borsbors 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

bors commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4c3e06a with merge a4d30a7...

@bors

bors commented Oct 27, 2020

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Oct 27, 2020
@bors
bors merged commit a4d30a7 into rust-lang:masterOct 27, 2020
@rustbotrustbot 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

Mark-Simulacrum commented Oct 27, 2020

Copy link
Copy Markdown
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

tmiasko commented Oct 27, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

Mark-Simulacrum commented Oct 27, 2020

Copy link
Copy Markdown
Member

Ah, ok. Makes sense. Thanks for investigating!

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

Reviewers

@wesleywiserwesleywiserwesleywiser approved these changes

Assignees

@wesleywiserwesleywiser

Labels

A-mir-optArea: MIR optimizationsmerged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: 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