-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make lowering a query #95573
Make lowering a query #95573
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 8f3583b024d77e73b2a2c6a05d89404fb2bad707 with merge 7c9d6056d06c28788c27759f69c7722558f629b2... |
☀️ Try build successful - checks-actions |
Queued 7c9d6056d06c28788c27759f69c7722558f629b2 with parent 297a801, future comparison URL. |
Finished benchmarking commit (7c9d6056d06c28788c27759f69c7722558f629b2): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led 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 Footnotes |
☔ The latest upstream changes (presumably #95662) made this pull request unmergeable. Please resolve the merge conflicts. |
I've done a first pass through the commits but I'll need a while to understand what's going on here |
The last commit should significantly simplify how the DepGraph understand the creation of new definitions. |
☔ The latest upstream changes (presumably #91557) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry for being slow to review this. I blocked off some time on Monday to do it. |
☔ The latest upstream changes (presumably #93803) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
OK, I've done another pass over the PR. It looks correct to me and it has many good improvements. Great work!
I'm not entirely happy with how special the handling of Definitions
is. But I think that's unavoidable in the short term. Longer term we can hopefully remove it entirely (at least in its current form).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This looks good to me now, @cjgillot. You can r=me if you think it's ready. |
@bors r=michaelwoerister |
☀️ Test successful - checks-actions |
Finished benchmarking commit (0f573a0): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Footnotes |
Results here look to be a slight regression, but overall I think are not worth further investigation -- some of the benchmarks are prone to noise over the last week or so (html5ever, tt-muncher in particular), and though other benchmarks are also affected they're not large enough that we should devote time to investigating in my opinion. |
Split from #88186.
This PR refactors the relationship between lowering and the resolver outputs in order to make lowering itself a query.
In a first part, lowering is changed to avoid modifying resolver outputs, by maintaining its own data structures for creating new
NodeId
s and so.Then, the
TyCtxt
is modified to allow creating newLocalDefId
s from inside it. This is done by:Definitions
in a lock, so as to allow modification;register_def
whose purpose is to declare aLocalDefId
to the query system.See
TyCtxt::create_def
andTyCtxt::iter_local_def_id
for more detailed explanations of the design.