Add some #[inline(always)] to arithmetic methods of integers #84061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
After this change this code: #[no_mangle]
pub fn method_b(a: u16){
let b = a as u32;
assert_eq!(b.wrapping_add(1), b+1);
}compiles into .def method_b;
.scl 2;
.type 32;
.endef
.section .text,"xr",one_only,method_b
.globl method_b
.p2align 4, 0x90
method_b:
retqwith -C opt-level=1. |
|
Do I need to add codegen tests for this? |
|
I wonder how this affects runtime benchmarks of opt-level=0 code in perf.rlo, i.e., procedural macros (for example const_ptr::offset accounts for ~0.3% instructions in cargo-check): @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit dee41768b96c73890aed7630342cb89bd75bb12d with merge 203fdd5ff68becff6518a8da1406562203cf5128... |
|
This seems okay to me, but I also wonder how the NewPM will change the behaviour we have right now. |
klensy
reviewed
Apr 10, 2021
library/core/src/ptr/const_ptr.rs
Outdated
| @@ -696,7 +696,6 @@ impl<T: ?Sized> *const T { | |||
| #[stable(feature = "pointer_methods", since = "1.26.0")] | |||
| #[must_use = "returns a new pointer rather than modifying its argument"] | |||
| #[rustc_const_unstable(feature = "const_ptr_offset", issue = "71499")] | |||
| #[inline] | |||
There was a problem hiding this comment.
Why this removed?
Typo, i guess, removed old and not inserted always.
There was a problem hiding this comment.
Thanks, I missed this.
|
☀️ Try build successful - checks-actions |
|
Queued 203fdd5ff68becff6518a8da1406562203cf5128 with parent 4029d4d, future comparison URL. |
|
@tmiasko We need run different perf because I accidentally deleted one of inlines in first commit :( |
|
Finished benchmarking try commit (203fdd5ff68becff6518a8da1406562203cf5128): 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 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 |
rustbot
removed
the
S-waiting-on-perf
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit f0c7308a8dff29848b863f7675c9a0c908d28425 with merge 8b83f733fef262fda5a9bda1873ca8070bc8531f... |
|
☀️ Try build successful - checks-actions |
|
Queued 8b83f733fef262fda5a9bda1873ca8070bc8531f with parent 28b948f, future comparison URL. |
|
Finished benchmarking try commit (8b83f733fef262fda5a9bda1873ca8070bc8531f): 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 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 |
rustbot
removed
the
S-waiting-on-perf
|
Looks like a noise for me. |
|
Isn't perf run on |
|
Cargo defaults to using -Copt-level=0 for host dependencies which includes procedural macros. As a result for crates that make have use of procedural macros you get profiles that look like one below (note for example cachegrind profile for cargo-check |
I tried to add it only to methods which return results of intrinsics and don't have any branching. Branching could made performance of debug builds (`-Copt-level=0`) worse. Main goal of changes is allowing wider optimizations in `-Copt-level=1`. Closes: #75598
|
Squashed commits. |
|
@bors r+ I don't see any reason to not do this. |
|
📌 Commit f8a12c6 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
AngelicosPhosphoros
deleted the
issue-75598-add-inline-always-arithmetic
branch
|
@AngelicosPhosphoros @nagisa perf regressions of 1.7% are not usually considered noise. We typically treat anything above 1% on non-volatile benchmarks as an issue that should at least be discussed. Given this is a relatively modest regression, I personally don't think this needs to be reverted, but I also think it is worth discussing whether the regressions in compile time are worth the potential optimizations. |
|
@rylev Sorry but I cannot find a regression. Could you tell exactly which benchmark regressed? |
|
@AngelicosPhosphoros If you look at the perf results you'll notice that instruction count on regex-debug full builds regressed by 1.7% which is definitely out of the range of change usually considered noise. It seems the It makes sense that codegen taking longer after more functions are inlined, so I'm not suggesting reverting this change. I just wanted to make sure we're doing the proper analysis on performance. It seems like this performance regression was shrugged off as noise, but it does not appear to be noise - this PR regresses some benchmarks compilation times. Again, IMO this regression is potentially worth the trade off of compile time regressions for runtime performance, but it would be nice to see that discussed a bit before merging. In short, there's no real action items here other than a call for a bit more care on performance analysis in the future. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2022
Make some trivial functions `#[inline(always)]` This is some kind of follow-up of PRs like rust-lang#85218, rust-lang#84061, rust-lang#87150. Functions that do very basic operations are made `#[inline(always)]` to avoid pessimizing them in debug builds when compared to using built-in operations directly.
RalfJung
pushed a commit
to RalfJung/miri
that referenced
this pull request
Dec 11, 2022
Make some trivial functions `#[inline(always)]` This is some kind of follow-up of PRs like rust-lang/rust#85218, rust-lang/rust#84061, rust-lang/rust#87150. Functions that do very basic operations are made `#[inline(always)]` to avoid pessimizing them in debug builds when compared to using built-in operations directly.
thomcc
pushed a commit
to tcdi/postgrestd
that referenced
this pull request
Feb 10, 2023
Make some trivial functions `#[inline(always)]` This is some kind of follow-up of PRs like rust-lang/rust#85218, rust-lang/rust#84061, rust-lang/rust#87150. Functions that do very basic operations are made `#[inline(always)]` to avoid pessimizing them in debug builds when compared to using built-in operations directly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I tried to add it only to methods which return results of intrinsics and don't have any branching.
Branching could made performance of debug builds (
-Copt-level=0) worse.Main goal of changes is allowing wider optimizations in
-Copt-level=1.Closes: #75598
r? @nagisa