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

Combine instructions immediately #81101

Merged
merged 2 commits into from
Jan 22, 2021
Merged

Combine instructions immediately #81101

merged 2 commits into from
Jan 22, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 16, 2021

No description provided.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jan 16, 2021
@petrochenkov
Copy link
Contributor

r? @lcnr

@rust-highfive rust-highfive assigned lcnr and unassigned petrochenkov Jan 17, 2021
@nagisa
Copy link
Member

nagisa commented Jan 17, 2021

This makes sense to me given the scope of instcombine today, but I imagine this would make it significantly more difficult to instcombine multiple statements into one if we ever get to the point of wanting to do so.

@lcnr
Copy link
Contributor

lcnr commented Jan 17, 2021

hmm, what @nagisa said makes sense to me.

Don't really have an opinion on if we should land this PR and am not involved enough to decide.

r? @nagisa feel free to reassign to someone else from wg-mir-opt though

@rust-highfive rust-highfive assigned nagisa and unassigned lcnr Jan 17, 2021
@nagisa
Copy link
Member

nagisa commented Jan 17, 2021

Thinking about it a little more:

Given the structure of the MIR, having to use the same pass for both multi-statement and single-statement combines is probably an overall negative, mostly because there is likely a significant inherent cost associated with maintaining state for inst-combines that operate over multiple MIR statements (in LLVM its fine because its all pointers and referencing instructions is very cheap due to its SSA nature).

So, it would make sense to me to have a separate pass for multi-statement instcombines once we start thinking about implementing those. On the other hand, in that world you'd probably want to run the two instcombines passes in a fixpoint anyway… so 🤷

cc @oli-obk

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 17, 2021

Non-local transformation would require a different approach, but as long as we don't make them this approach has a benefit of making it clear that one transformation cannot invalidate another, since transforms are immediate and stateless.

This is relatively trivial amount of code so we can always change this as a need arises.

@bors
Copy link
Contributor

bors commented Jan 18, 2021

☔ The latest upstream changes (presumably #80865) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa
Copy link
Member

nagisa commented Jan 22, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2021

📌 Commit 508eec4 has been approved by nagisa

@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 Jan 22, 2021
@nagisa
Copy link
Member

nagisa commented Jan 22, 2021

@bors rollup=never

@bjorn3
Copy link
Member

bjorn3 commented Jan 22, 2021

Should this get a perf run?

@nagisa
Copy link
Member

nagisa commented Jan 22, 2021

I'm pretty confident this will be a net win due to the new code just doing less work walking the MIR structure. I still made it rollup=never so that we do eventually see the perf results once this lands, though.

@bors
Copy link
Contributor

bors commented Jan 22, 2021

⌛ Testing commit 508eec4 with merge f2de221...

@bors
Copy link
Contributor

bors commented Jan 22, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing f2de221 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 22, 2021
@bors bors merged commit f2de221 into rust-lang:master Jan 22, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 22, 2021
@bjorn3
Copy link
Member

bjorn3 commented Jan 23, 2021

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 23, 2021

The perf results should show measurement error based on historical data, they are quite misleading otherwise. For example ctfe-stress-4-check which shows 1.3% slowdown never executes this code. Similarly debug and opt variants of this benchmark execute it only once on a MIR body with a single statement.

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.

8 participants