-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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.
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
Tweak assert_unsafe_precondition helpers so they optimize well in MIR #105114
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit cdfcdde2b454efef14937007c379bc4b761140cb with merge fabf9d669d8708f8b2b4fd8ea76aa78c39348c58... |
Please add some mir opt tests showing that these changes actually affect anything. Wrt diverging functions: we need to make sure that we never inline |
Currently |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (fabf9d669d8708f8b2b4fd8ea76aa78c39348c58): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 22060a4a18da19b496d6a632ade9af515ce98c54 with merge 8d3e345aa88d91f3e68460bb35b887de6e5646c1... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (8d3e345aa88d91f3e68460bb35b887de6e5646c1): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
22060a4
to
07f1631
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 07f1631 with merge 4456381d0fd5bc731b26ab2c2579b25810be54e3... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (4456381d0fd5bc731b26ab2c2579b25810be54e3): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit a4b5bfe with merge 65a3ba24089752ff8febf5794f60d05bd8af3686... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (65a3ba24089752ff8febf5794f60d05bd8af3686): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
I've changed direction here, closing this so people don't get notifications for something they didn't sign up for. |
It turns out that #104121 does not actually achieve what @Lokathor wanted, when debug assertions are enabled. In my opinion there are really multiple issues here, this PR addresses all of them:
The implementation of
is_aligned_and_not_null
is compiled to a shocking amount of MIR, because the implementation ofptr::is_null
is contorted to work inconst fn
, and the implementation ofptr::is_aligned
is based onptr::is_aligned_to
which among other things has a check that the alignment is a power of 2. These of course clean up quite nicely in LLVM... but the goal here is to enable inlining before we get to LLVM.Then some of the helper functions aren't
#[inline]
. The MIR inliner only inlines#[inline]
functions at-Zmir-opt-level=2
, which is what--release
corresponds to. Whether or not that heuristic should be loosened up, these are good candidates for inlining so I don't see any harm in adding the attribute.Then the MIR inlining cost estimation seems a bit pessimistic to me. The MIR we generate tends to have a lot of assignments from locals to locals. By eye it looks like a lot of these could be erased by another optimization pass, so I don't think they represent nearly the complexity of any other instruction. For now, this excludes them from cost estimation entirely.
I also removed the extra function call penalty from diverging call terminators. My logic there is that a diverging terminator is often a panic path, and those sometimes (and especially in this case) have a predicate which can be statically reasoned about. There is a fair chance that inlining a function with a diverging terminator will result in optimizing away the divergence entirely, or making other optimizations after the diverging path, based on assume its predicate. (I'm aware that currently MIR opt is not very good at this sort of cleanup, so this is somewhat of a gamble)
(I hope this looks good in perf...)
r? @cjgillot
cc @JakobDegen