Use assume to inform the optimiser about refcount invariants #21418
Conversation
The reference count can never be 0, unless we're about to drop the data completely. Using the `assume` intrinsic allows us to inform LLVM about that invariant, meaning it can avoid unnecessary drops.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. 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 CONTRIBUTING.md for more information. |
|
Nice win! It seems like there's still room for improvement as there's still a check for null, so perhaps this shouldn't close #13018 but rather just update the IR? |
|
@alexcrichton Oh, I missed that. I should be able to tell it to assume that the pointer is non-null. |
|
Ok, so I updated this to add assumptions that pointer is non-null in a couple places. New IR: https://gist.github.com/Aatch/3786d20df2edaad6a0e8#file-rc-after2-ll That has some junk that isn't required because it's only there for the |
|
r=me with some comments on each Nice find! |
The reference count can never be 0, unless we're about to drop the data completely. Using the `assume` intrinsic allows us to inform LLVM about that invariant, meaning it can avoid unnecessary drops. --- Before and after IR: https://gist.github.com/Aatch/3786d20df2edaad6a0e8 Generated from the example in rust-lang#13018 Fixes rust-lang#13018
|
If the rollup bounces again I may have to back this out, it looks like we've taken a serious hit in compile times based on this patch. I did a Spending 10% of our time in computing knowledge from assumptions seems like quite a lot! |
|
To put this into concrete numbers: |
|
Ugh, ouch. That's surprising, but not entirely unexpected. The assumption tracker framework is relatively new in LLVM, so it's not too surprising that it hasn't been tuned as much as it could be. I'm fine with pulling the PR if it has that much of a negative effect on compile times. |
|
Could you measure the impact on just having the assumptions on reference counting? The assumptions on |
|
Ah sorry this didn't actually merge (had to revert the commits). Feel free to make a new PR though! |
This is half of what @Aatch implemented in #21418. The non-null assumption is later canonicalized to !nonnull metadata and doesn't cause any slowdowns (in fact the build is slightly faster with this change). I left out the other half of #21418 because that still causes a ~16% increase in compile times (30m -> 35m).
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).
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).
The reference count can never be 0, unless we're about to drop the data
completely. Using the
assumeintrinsic allows us to inform LLVM aboutthat invariant, meaning it can avoid unnecessary drops.
Before and after IR: https://gist.github.com/Aatch/3786d20df2edaad6a0e8
Generated from the example in #13018
Fixes #13018