-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Create definitions for promoted constants. #111693
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #111568) made this pull request unmergeable. Please resolve the merge conflicts. |
Does it make sense to remove |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit ece7178 with merge 9544b34e922aa47c0406b4eeb76fe0937973d406... |
This comment has been minimized.
This comment has been minimized.
We stop borrow-checking promoted as part of the enclosing body, so they keep being checked only once. They now work exactly like inline consts. However, we now run the rest of borrow-checking on them, and pay the extra cost of setting-up and cleaning-up the borrow-checking environment: renumbering regions, solving them, and wrapping that up into a nice Profiling that led me to #111753 and #111759. This PR's perf will have to be re-checked on those land. |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 6ace5ea with merge 42412266728f075fe25b6499de0e33bc656748b9... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (42412266728f075fe25b6499de0e33bc656748b9): 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: 643.295s -> 644.706s (0.22%) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit d07bb8e with merge c8983833ea528097b0bfc7fb2b3b267dc37abfa3... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c8983833ea528097b0bfc7fb2b3b267dc37abfa3): 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: 641.899s -> 645.473s (0.56%) |
The performance degradation in incremental is due to a current limitation of creating new definitions. When we create a new definition, we make the creator query depend on the forever-red dep-node. This means that in the incremental scenario, we force There is also a perf degradation in non-incremental, inside borrowck. I haven't found its cause yet. |
Closing this as it was an experiment and has too many conflicts |
Since I forgot to cross link it: I'm working on fixing the perf issues here via #115613 |
The current handling of promoted carries an
Option<Promoted>
everywhere to disambiguate the normal item from the constant.This PR replaces the promoted constants by new definitions with new
DefKind::Promoted
.This method allows to unify borrow-checking with the code path for inline consts.
The main caveat is creating even more definitions that do not have any HIR associated, triggering new corner cases where
local_def_id_to_hir_id
panics.