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

At opt-level=0, apply only ABI-affecting attributes to functions #94127

Merged
merged 4 commits into from
Feb 26, 2022
Merged

At opt-level=0, apply only ABI-affecting attributes to functions #94127

merged 4 commits into from
Feb 26, 2022

Conversation

erikdesjardins
Copy link
Contributor

This should provide a small perf improvement for debug builds,
and should more than cancel out the perf regression from adding noundef (#93670 (comment), #94106).

r? @nikic

@erikdesjardins
At opt-level=0, apply only ABI-affecting attributes to functions
dcbdc8c 
This should provide a small perf improvement for debug builds,
and should more than cancel out the regression from adding noundef,
which was only significant in debug builds.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 18, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2022
@erikdesjardins erikdesjardins mentioned this pull request Feb 18, 2022
Closed
@lqd
Copy link
Member

lqd commented Feb 18, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2022
@bors
Copy link
Contributor

bors commented Feb 18, 2022

⌛ Trying commit dcbdc8c with merge 3515289ab467c3593651fc70949029eba87453b3...

@bors
Copy link
Contributor

bors commented Feb 18, 2022

☀️ Try build successful - checks-actions
Build commit: 3515289ab467c3593651fc70949029eba87453b3 (3515289ab467c3593651fc70949029eba87453b3)

@rust-timer
Copy link
Collaborator

Queued 3515289ab467c3593651fc70949029eba87453b3 with parent b8c56fa, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3515289ab467c3593651fc70949029eba87453b3): comparison url.

Summary: This benchmark run shows 49 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -1.4%
  • Largest improvement in instruction counts: -4.3% on incr-patched: Job builds of regex debug

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2022
@nikic
Copy link
Contributor

nikic commented Feb 19, 2022

It may be worthwhile to split our LLVM attribute APIs into ones that create an Attribute, an then another that applies a whole list of attributes to an index. What we're doing right now if really inefficient and will end up interning a lot of unnecessary AttributeSets and AttributeLists.

@erikdesjardins
Copy link
Contributor Author

I'll work on that next, after my current batch of PRs.

Unless you're suggesting that it would make our attribute manipulation fast enough that this PR would no longer be necessary?

@erikdesjardins erikdesjardins mentioned this pull request Feb 19, 2022
Merged
@nikic
Copy link
Contributor

nikic commented Feb 19, 2022

Unless you're suggesting that it would make our attribute manipulation fast enough that this PR would no longer be necessary?

I suspect so, but I don't particularly mind the conditional attributes either.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2022

📌 Commit 6e740ae has been approved by nikic

@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 Feb 19, 2022
@erikdesjardins erikdesjardins mentioned this pull request Feb 21, 2022
Merged
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Testing commit 6e740ae with merge 47b2057e8ae367c39665eff9045230bfb2c9e114...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 24, 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 Feb 24, 2022
@nikic
Copy link
Contributor

nikic commented Feb 25, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2022

📌 Commit b0921f8 has been approved by nikic

@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 Feb 25, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

⌛ Testing commit b0921f8 with merge bab0f5360a62cdad0937140a441e1416ff807834...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 25, 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 Feb 25, 2022
@nikic
Copy link
Contributor

nikic commented Feb 26, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2022

📌 Commit 945276c has been approved by nikic

@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 Feb 26, 2022
@bors
Copy link
Contributor

bors commented Feb 26, 2022

⌛ Testing commit 945276c with merge 8128e91...

@bors
Copy link
Contributor

bors commented Feb 26, 2022

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 8128e91 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 26, 2022
@bors bors merged commit 8128e91 into rust-lang:master Feb 26, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 26, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8128e91): comparison url.

Summary: This benchmark run shows 39 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -1.2%
  • Largest improvement in instruction counts: -3.9% on incr-patched: Job builds of regex debug

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

@rustbot label: -perf-regression

@erikdesjardins erikdesjardins deleted the debugattr branch February 26, 2022 15:54
@erikdesjardins erikdesjardins mentioned this pull request Feb 26, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2022
@bors
Auto merge of rust-lang#94221 - erikdesjardins:addattr, r=nikic
2bd9656 
Add LLVM attributes in batches instead of individually

This should improve performance.

~r? `@ghost` (blocked on rust-lang#94127)~
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2022
@bors
Auto merge of rust-lang#94157 - erikdesjardins:more-noundef, r=nikic
9fbff89 
Apply noundef attribute to all scalar types which do not permit raw init

Beyond `&`/`&mut`/`Box`, this covers `char`, enum discriminants, `NonZero*`, etc.
All such types currently cause a Miri error if left uninitialized,
and an `invalid_value` lint in cases like `mem::uninitialized::<char>()`.

Note that this _does not_ change whether or not it is UB for `u64` (or
other integer types with no invalid values) to be undef.

Fixes (partially) rust-lang#74378.

r? `@ghost` (blocked on rust-lang#94127)

`@rustbot` label S-blocked
@erikdesjardins erikdesjardins mentioned this pull request Feb 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

Assignees

@nikic nikic

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
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
8 participants
@erikdesjardins @lqd @rust-timer @bors @nikic @rust-log-analyzer @rust-highfive @rustbot