Change Rc::inc_{weak,strong} to better hint optimization to LLVM
#53080
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/liballoc/rc.rs
Outdated
| // The reference count will never be zero when this is called; | ||
| // nevertheless, we insert an abort here to hint LLVM at | ||
| // an otherwise missied optimization. | ||
| if self.strong() == 0 || self.strong() == usize::max_value() { |
There was a problem hiding this comment.
Hm, should we instead have unreachable_unchecked for the == 0 case?
There was a problem hiding this comment.
Confusingly, doing this:
- if self.strong() == 0 || self.strong() == usize::max_value() {
+ if self.strong() == 0 {
+ unsafe { unreachable_unchecked(); }
+ }
+ if self.strong() == usize::max_value() {
unsafe { abort(); }produces the same output as before any changes (before in the linked comparison).
There was a problem hiding this comment.
Oh, hm, that might be because LLVM for whatever reason decides that subtraction can overflow or some such. Seems fine to leave it then.
src/liballoc/rc.rs
Outdated
| // We want to abort on overflow instead of dropping the value. | ||
| // The reference count will never be zero when this is called; | ||
| // nevertheless, we insert an abort here to hint LLVM at | ||
| // an otherwise missied optimization. |
There was a problem hiding this comment.
Typo: “missied” (here and below).
|
Ping from triage @aidanhs. This PR requires your review. |
|
r? @rkruppe or @alexcrichton |
|
Awesome, thanks for this @hermord! Out of curiosity, how come there's a check for 0 here? I don't think the code currently checks that, right? Additionally, it'd be awesome if we could add a codegen test for this! I'm not sure how easy it would be to do so though. |
|
@alexcrichton The code currently doesn't check for it, but it's needed to hint LLVM that the |
|
The extra check doesn't cause any regression (at least on x86) since both checks get lowered to a single |
|
Ok looks and sounds great to me, thanks! @bors: r+ |
|
📌 Commit 0b83914 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- You have accidentally changed the |
bors
added
S-waiting-on-author
|
You might be able to undo the problem by doing |
|
Oops, my apologies. That's what I get for doing |
|
Fixed now. |
|
@bors: r+ |
|
📌 Commit 0dd10af has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 0dd10af with merge d43445d36457be55114d64ba93025cec9fc07ad8... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors: r+ |
|
📌 Commit 79a905e has been approved by |
bors
added
S-waiting-on-bors
Change `Rc::inc_{weak,strong}` to better hint optimization to LLVM
As discussed in #13018, `Rc::inc_strong` and `Rc::inc_weak` are changed to allow compositions of `clone` and `drop` to be better optimized. Almost entirely as in [this comment](#13018 (comment)), except that `abort` on zero is added so that a `drop(t.clone())` does not produce a zero check followed by conditional deallocation.
This is different from #21418 in that it doesn't rely on `assume`, avoiding the prohibitive compilation slowdown.
[Before and after IR](https://gist.github.com/hermord/266e55451b7fe0bb8caa6e35d17c86e1).
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Huh, |
bors
added
S-waiting-on-bors
Change `Rc::inc_{weak,strong}` to better hint optimization to LLVM
As discussed in #13018, `Rc::inc_strong` and `Rc::inc_weak` are changed to allow compositions of `clone` and `drop` to be better optimized. Almost entirely as in [this comment](#13018 (comment)), except that `abort` on zero is added so that a `drop(t.clone())` does not produce a zero check followed by conditional deallocation.
This is different from #21418 in that it doesn't rely on `assume`, avoiding the prohibitive compilation slowdown.
[Before and after IR](https://gist.github.com/hermord/266e55451b7fe0bb8caa6e35d17c86e1).
|
☀️ Test successful - status-appveyor, status-travis |
As discussed in #13018,
Rc::inc_strongandRc::inc_weakare changed to allow compositions ofcloneanddropto be better optimized. Almost entirely as in this comment, except thataborton zero is added so that adrop(t.clone())does not produce a zero check followed by conditional deallocation.This is different from #21418 in that it doesn't rely on
assume, avoiding the prohibitive compilation slowdown.Before and after IR.