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

Enable function merging when opt is for size #100035

Merged
merged 1 commit into from
Aug 6, 2022

Conversation

workingjubilee
Copy link
Member

It is, of course, natural to want to merge aliasing functions when
optimizing for code size, since that can eliminate several bytes.
And an exhaustive match helps make the code less brittle.

Closes #98215.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 1, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Aug 1, 2022
@michaelwoerister
Copy link
Member

Thanks for the PR, @workingjubilee!

@rust-lang/wg-llvm, is there a chance that this increases the risk of miscompilations for -Copt-level=s and -Copt-level=z? I seem to remember that these optimization levels were buggier in LLVM than the "regular" ones (at least for -Copt-level=z).

Otherwise, this looks good to me.

@nikic
Copy link
Contributor

nikic commented Aug 3, 2022

I don't think this meaningfully increases the risk of miscompilations, as we're already using MergeFunctions for other optimization levels. (Clang does not use the pass by default, so it does receive less upstream testing than would be ideal, but I don't think size optimizations are meaningfully different in this regard.)

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 3, 2022

📌 Commit 1def1cecb9d4310e51266358126e2895f36de574 has been approved by nikic

It is now in the queue for this repository.

@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 Aug 3, 2022
@bors
Copy link
Contributor

bors commented Aug 4, 2022

⌛ Testing commit 1def1cecb9d4310e51266358126e2895f36de574 with merge 4f607ea92e73e9b39a4a8c7ebd4a16a22d3ef80f...

@bors
Copy link
Contributor

bors commented Aug 4, 2022

💔 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 Aug 4, 2022
@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member

@bors retry

@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 Aug 5, 2022
@bors
Copy link
Contributor

bors commented Aug 5, 2022

⌛ Testing commit 1def1cecb9d4310e51266358126e2895f36de574 with merge 8653ab47796c93b3e3b3c7ac2657a88a8fc489d4...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 5, 2022

💔 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 Aug 5, 2022
@workingjubilee
Copy link
Member Author

huh, I could have sworn I tested that locally and it passed...? But now that I poke it, it perturbs fairly easily. I still want to test that -Zmerge-functions works on its own, while we have it, so I guess the only safe bet is if the source is an exact copy.

@nikic
Copy link
Contributor

nikic commented Aug 5, 2022

@workingjubilee Well, at least as implemented -Zmerge-functions=aliases doesn't do anything without optimizations. The optimization check always happens, -Zmerge-functions does not override it. So I'd expect this to still fail if the test gets run with -C opt-level=0.

@workingjubilee
Copy link
Member Author

Oh hm. Then... it's odd that it passed at all...?

It is, of course, natural to want to merge aliasing functions when
optimizing for code size, since that can eliminate several bytes.
And an exhaustive match helps make the code less brittle.
@workingjubilee
Copy link
Member Author

Going to resist the temptation to get rabbitholed and simply remove all testing for -Zmerge-functions=aliases specifically for now, will investigate that point of curiosity another day.

@nikic
Copy link
Contributor

nikic commented Aug 5, 2022

Oh hm. Then... it's odd that it passed at all...?

The tests run with -O by default, which is why the failure only showed up on the nopt job.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 5, 2022

📌 Commit 80c9012 has been approved by nikic

It is now in the queue for this repository.

@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 Aug 5, 2022
@bors
Copy link
Contributor

bors commented Aug 5, 2022

⌛ Testing commit 80c9012 with merge 55f4641...

@bors
Copy link
Contributor

bors commented Aug 6, 2022

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 55f4641 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2022
@bors bors merged commit 55f4641 into rust-lang:master Aug 6, 2022
@rustbot rustbot added this to the 1.64.0 milestone Aug 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (55f4641): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
2.0% 2.0% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.7% -2.7% 1
All 😿🎉 (primary) 2.0% 2.0% 1

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@workingjubilee workingjubilee deleted the merge-functions branch August 6, 2022 07:58
@jrose-signal
Copy link

I don't think this meaningfully increases the risk of miscompilations, as we're already using MergeFunctions for other optimization levels. (Clang does not use the pass by default, so it does receive less upstream testing than would be ideal, but I don't think size optimizations are meaningfully different in this regard.)

FWIW Swift does use MergeFunctions by default, so there's not zero production testing. Of course, Swift uses its own fork of LLVM, so issues could still creep in.

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"-Z merge-functions" does not enable the MergeFunctions pass under opt-level=s
10 participants