-
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
Encode MIR metadata by iterating on DefId instead of traversing the HIR tree #81215
Conversation
(true, false) | ||
} | ||
// Closures and functions | ||
DefKind::Generator | DefKind::Closure | DefKind::AssocFn | DefKind::Fn => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently optimized MIR for generators is always required. I will have to revisit the tests if this implementation manages to pass them. EDIT: It doesn't.
The job Click to see the possible cause of the failure (guessed by this bot)
|
I don't have time to review before mid next week. @tmiasko do you want to take this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I really like that now we have a single function responsible for deciding what to encode that can be reused between prefetcher & encoder. Additionally with MIR being encoded separately, we could easily include its size in statistics.
The change in diagnostics is probably due to the iteration order in |
If the iteration order were stable, then the exact order would be irrelevant. But I'm guessing this order depends on a hash map. We would get spurious changes in ui tests failing all the time if we are dependent on a hashmap for diagnostics ordering. |
Last commit sorts the hash map to avoid the unstable iteration in diagnostics. |
☔ The latest upstream changes (presumably #80919) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased with the test back-and-forth removed. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 5a60f0a with merge f7291779fd3275697027f6eefb933af7f154c92b... |
☀️ Try build successful - checks-actions |
Queued f7291779fd3275697027f6eefb933af7f154c92b with parent b593389, future comparison URL. |
Finished benchmarking try commit (f7291779fd3275697027f6eefb933af7f154c92b): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Similar results. |
Well... the CTFE stress test regression is completely gone. So that's good. Now we're seeing a |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 09ac459 with merge d015793ce5cbbc0e7555778a818058a8d558a7ab... |
☀️ Try build successful - checks-actions |
Queued d015793ce5cbbc0e7555778a818058a8d558a7ab with parent e708cbd, future comparison URL. |
Finished benchmarking try commit (d015793ce5cbbc0e7555778a818058a8d558a7ab): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
The clap regression does not appear any more. |
In wg-grammar some constructors are no longer queried in 2,90d1
< rust_grammar.parse-_C-Expr__0-{constructor#0}.-------.mir_map.0.mir
< rust_grammar.parse-_C-Expr__10__0-{constructor#0}.-------.mir_map.0.mir
< rust_grammar.parse-_C-Expr__10__1-{constructor#0}.-------.mir_map.0.mir
...
< rust_grammar.parse-_C-TOKEN_TREE-{constructor#0}.-------.mir_map.0.mir
17913,17997d17823
< rust_grammar.parse-ListHead-Nil-{constructor#0}.-------.mir_map.0.mir
< rust_grammar.parse-_P-_0-{constructor#0}.-------.mir_map.0.mir
...
< rust_grammar.parse-_P-_9-{constructor#0}.-------.mir_map.0.mir
18088d17913
< rust_grammar.parse-ParseError-NoParse-{constructor#0}.-------.mir_map.0.mir |
It turns out we encoded MIR for fictive constructors previously. |
Ok, I went over the leftover "regressions" (in the 0.x % range), and all of them seem to be because there's an additional call to @bors r+ |
📌 Commit 09ac459 has been approved by |
☀️ Test successful - checks-actions |
Split out of #80347.
This part only traverses
mir_keys
and encodes MIR according to the def kind.r? @oli-obk