-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
restrict promotion of const fn
calls
#121557
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@bors try |
…<try> restrict promotion of `const fn` calls We only promote them in `const`/`static` initializers, but even that is still unfortunate -- we still cannot add promoteds to required_consts. But we should add them there to make sure it's always okay to evaluate every const we encounter in a MIR body. That effort of not promoting things that can fail to evaluate is tracked in rust-lang#80619. These `const fn` calls are the last missing piece. So I propose that we do not promote const-fn calls in const when that may fail without the entire const failing, thereby completing rust-lang#80619. Unfortunately we can't just reject promoting these functions outright due to backwards compatibility. So let's see if we can find a hack that makes crater happy... Attempt one: only promote calls on the "safe path" at the beginning of a MIR block. This is the path that starts at the start block and continues via gotos and calls, but stops at the first branch. r? `@oli-obk`
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
98c587a
to
55dbcde
Compare
@rust-timer build 80d5fe3 |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (80d5fe3): 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.
Binary sizeResultsThis 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.
Bootstrap: 648.482s -> 651.592s (0.48%) |
Hm, I guess that has to be the extra entries in I am going to worry about perf once crater looks good. |
☔ The latest upstream changes (presumably #121569) made this pull request unmergeable. Please resolve the merge conflicts. |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
c2e6a9e
to
8f4d144
Compare
Ah, I screwed up the logic and now it no longer promotes @bors try |
💔 Test failed - checks-actions |
85433c4
to
5237c33
Compare
then we can also make all_required_consts_are_checked a constant instead of a function
5237c33
to
8436045
Compare
@bors r=oli-obk rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (fee0e66): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression 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)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 674.182s -> 675.372s (0.18%) |
We only promote them in
const
/static
initializers, but even that is still unfortunate -- we still cannot add promoteds to required_consts. But we should add them there to make sure it's always okay to evaluate every const we encounter in a MIR body. That effort of not promoting things that can fail to evaluate is tracked in #80619. Theseconst fn
calls are the last missing piece.So I propose that we do not promote const-fn calls in const when that may fail without the entire const failing, thereby completing #80619. Unfortunately we can't just reject promoting these functions outright due to backwards compatibility. So let's see if we can find a hack that makes crater happy...
For the record, this is the crater analysis from when I tried to entirely forbid this kind of promotion. It's a tiny amount of breakage and if we had a nice alternative for code like that, we could conceivably push it through... but sadly, inline const expressions are still blocked on t-lang concerns about post-monomorphization errors and we haven't yet figured out an implementation that can resolve those concerns. So we're forced to make progress via other means, such as terrible hacks like this.
Attempt one: only promote calls on the "safe path" at the beginning of a MIR block. This is the path that starts at the start block and continues via gotos and calls, but stops at the first branch. If we had imposed this restriction before stabilizing
if
andmatch
inconst
, this would have definitely been sufficient...EDIT: Turns out that works. :)
Here's the t-lang nomination comment. And here's the FCP comment.
r? @oli-obk