Optimise fast path of checked_ops with unlikely
#73938
Conversation
|
r? @nagisa |
|
Do you have unit tests that enforce that the resulting asm contains the correct happy path? |
The most rustc could do is to generate |
|
rustc is a compiler, and a compiler must have ways to verity what kind of asm it generates. If you change something in the compiler with the purpose of changing the resulting asm happy path, and you can't be sure it produces the desired result, what's the point in changing the compiler? Probably I am missing something. (One thing I'm missing: you're chanting the std lib, not the compiler) |
rustc lowers Rust code to LLVM IR and let LLVM generates the assembly. Rust is not pinned to any particular version of LLVM so the assembly it generates may and will change over time. The most we could do is to ensure that
The change made here is not in the compiler, but the standard library (libcore to be precise). Furthermore, the change made here hints LLVM to prefer the fast path, but there is no guarantee that LLVM will take advantage of that. |
|
@bors r+ |
|
📌 Commit 86d8644 has been approved by |
bors
added
the
S-waiting-on-bors
…arth Rollup of 10 pull requests Successful merges: - rust-lang#73414 (Implement `slice_strip` feature) - rust-lang#73564 (linker: Create GNU_EH_FRAME header by default when producing ELFs) - rust-lang#73622 (Deny unsafe ops in unsafe fns in libcore) - rust-lang#73684 (add spans to injected coverage counters, extract with CoverageData query) - rust-lang#73812 (ast_pretty: Pass some token streams and trees by reference) - rust-lang#73853 (Add newline to rustc MultiSpan docs) - rust-lang#73883 (Compile rustdoc less often.) - rust-lang#73885 (Fix wasm32 being broken due to a NodeJS version bump) - rust-lang#73903 (Changes required for rustc/cargo to build for iOS targets) - rust-lang#73938 (Optimise fast path of checked_ops with `unlikely`) Failed merges: r? @ghost
| @@ -745,7 +760,7 @@ $EndFeature, " | |||
| #[inline] | |||
| pub const fn checked_add(self, rhs: Self) -> Option<Self> { | |||
| let (a, b) = self.overflowing_add(rhs); | |||
| if b {None} else {Some(a)} | |||
| if unlikely!(b) {None} else {Some(a)} | |||
There was a problem hiding this comment.
Will it have the same effect by flipping the case?
if !b { Some(a) } else { None }There was a problem hiding this comment.
Rearranging the code gives no hint to LLVM that one branch has more weight than another.
There was a problem hiding this comment.
I tried it. The code is better for only this method alone, not at callsite. But at callsite, things are not improved.
Yes, that's the same thing you can infer from the asm I posted here: That's why I have asked for some kind of unittests that this change actually does something useful... |
I think @lzutao is talking about the rearranged code suggested by @pickfire rather than this PR. As I mentioned before, the most we could do is to pass the hint to LLVM, and we cannot guarantee that LLVM will take advantage of that. |
This PR marks paths returning
Nonein checked_ops as unlikely to improvde codegen.Fixes #73731