Skip to content

Do not count inline(always) in inlining depth control #108788

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
Closed
wants to merge 4 commits into from
Closed

Do not count inline(always) in inlining depth control #108788

wants to merge 4 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Mar 5, 2023

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 5, 2023
@cjgillot
Copy link
Contributor Author

cjgillot commented Mar 5, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 5, 2023
@bors
Copy link
Collaborator

bors commented Mar 5, 2023

⌛ Trying commit b9af0eed87a6ed5896be4e8c64ec327ea62025ad with merge ed8f2bd542b391447effacefb3f9a68e93d61e09...

scottmcm
scottmcm reviewed Mar 5, 2023
View reviewed changes
library/core/src/cmp.rs
@@ -1484,11 +1487,11 @@ mod impls {
where
A: ~const PartialEq<B>,
{
#[inline]
#[inline(always)]
Copy link
Member

Choose a reason for hiding this comment

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

Since inlining is bottom-up, always is usually not desirable on generic things, since it will inline-always the whole eq implementation if it happens to get inlined into this one.

These situations really want some kind of inline(early) that would inline the wrappers at MIR level without telling LLVM to alwaysinline them.

scottmcm
scottmcm reviewed Mar 5, 2023
View reviewed changes
library/core/src/cmp.rs
@@ -1391,17 +1391,17 @@ mod impls {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_cmp", issue = "92391")]
impl const PartialOrd for $t {
#[inline]
#[inline(always)]
fn partial_cmp(&self, other: &$t) -> Option<Ordering> {
Some(self.cmp(other))
Copy link
Member

Choose a reason for hiding this comment

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

This is dozens of lines of MIR, due to being a bunch of primitive operations and switches (https://rust.godbolt.org/z/jaazhve7E), so it's not clear to me that it should be always'd.

It probably wants #105840 or a spaceship intrinsic to cut down the MIR size before doing it always.

@bors
Copy link
Collaborator

bors commented Mar 5, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@scottmcm scottmcm mentioned this pull request Mar 6, 2023
Closed
scottmcm added a commit to scottmcm/rust that referenced this pull request Mar 6, 2023
@scottmcm
Experiment: spaceship in MIR
d6fa338 
As mentioned in <rust-lang#108788 (comment)>, `Ord::cmp` on primitives generates a large amount of MIR, preventing (or at least discouraging) it from mir-inlining.

Let's see whether making it a MIR primitive helps stuff -- derived `(Partial)Ord` in particular, if we're lucky.
@cjgillot
Copy link
Contributor Author

cjgillot commented Mar 6, 2023

@rust-timer build ed8f2bd542b391447effacefb3f9a68e93d61e09

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ed8f2bd542b391447effacefb3f9a68e93d61e09): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.8% [0.6%, 1.1%] 3
Regressions ❌
(secondary)		 1.2% [0.3%, 2.6%] 13
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 2
All ❌✅ (primary) 0.8% [0.6%, 1.1%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.5% [-2.8%, -0.2%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.5% [-2.8%, -0.2%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 6, 2023
@Dylan-DPC Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2023
@bors
Copy link
Collaborator

bors commented Jun 14, 2023

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 14, 2023
@cjgillot cjgillot closed this Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

Assignees
No one assigned
Labels
perf-regression Performance regression. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjgillot @rust-timer @bors @scottmcm @rustbot @Dylan-DPC