Skip to content
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 AST->HIR lowering incremental #88186

Closed
wants to merge 16 commits into from
Closed

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Aug 20, 2021

Implementation of MCP rust-lang/compiler-team#452

AST->HIR lowering is currently done ahead of the construction of the incremental compilation engine. This issue proposes to make lowering incremental by introducing a query lower_to_hir(def: LocalDefId) -> Option<&hir::OwnerInfo<'_>> which lowers the AST item corresponding to definition def into the corresponding HIR and some associated information.

The objective is to eventually skip lowering some parts of the AST, and to allow progress towards making resolution and expansion incremental.

The proposed design is as follows:

  • remove global analyses from the current lowering pass, and pull them back to resolution stage;
  • attach lowering information to definitions instead of having multiple maps inside hir::Crate, effectively making hir::Crate an IndexVec<LocalDefId, Option<OwnerInfo<'_>>>;
  • extract the required resolution outputs from the resolver and keep them immutable;
  • allow to create new definitions from inside the query system (*);
  • index the AST by LocalDefId, using an IndexVec<LocalDefId, AstOwner> where AstOwner is an enum holding OwningRef<Lrc<Crate>, ItemLike> for ItemLike different AST item types;
  • perform HIR indexing per-owner;
  • make lowering incremental.

One of the most subtle changes is (*), which allows to create new definitions from inside queries. It is made safe using an eval-always query which forces re-execution of its caller, and effectively enforces the side-effects creating the definition. The first time definitions are iterated over (for instance for metadata output), a read-lock to the definitions is leaked, which seals the list of definitions.

This PR is blocked on:

The query scheme should basically look like this:

  • the untracked ast is used to compute lower_to_hir;
  • lower_to_hir yields hir_attrs, hir_owner_nodes and hir_owner_parent;
  • hir_owner_nodes yields hir_owner;
  • hir_module_items uses hir_owner_nodes to gather all the items inside a module;
  • hir_crate uses hir_owner_nodes to gather all the items inside the crate.

r? @michaelwoerister as requested on Zulip

@cjgillot cjgillot added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Aug 20, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2021
@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 29, 2021
@bors
Copy link
Contributor

bors commented Aug 29, 2021

⌛ Trying commit aa6204b46dcb5a45bc915430bfe05975d4ae38ea with merge f5e4230e3edf7c9689e3a4b9fde9426a3f7630ff...

@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 29, 2021

☀️ Try build successful - checks-actions
Build commit: f5e4230e3edf7c9689e3a4b9fde9426a3f7630ff (f5e4230e3edf7c9689e3a4b9fde9426a3f7630ff)

@rust-timer
Copy link
Collaborator

Queued f5e4230e3edf7c9689e3a4b9fde9426a3f7630ff with parent 59ce765, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (f5e4230e3edf7c9689e3a4b9fde9426a3f7630ff): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -4.9% on full builds of cranelift-codegen)
  • Very large regression in instruction counts (up to 26.5% on incr-unchanged builds of match-stress-enum)

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 perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 29, 2021
@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot marked this pull request as draft September 18, 2021 09:44
@bors

This comment has been minimized.

@cjgillot
Copy link
Contributor Author

cjgillot commented Apr 2, 2022

This PR features a sizeable perf regression, up to 11%. I believe this is entirely due to query overhead: this PR requires one extra query invocation for each LocalDefId in the crate. I split #95573 which makes lowering a query, but does not separate lowering each LocalDefId.

@cjgillot cjgillot mentioned this pull request Apr 2, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

☔ The latest upstream changes (presumably #95662) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

I'm setting this PR to blocked for now. #95573 looks promising!

@michaelwoerister michaelwoerister removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2022
@bors
Copy link
Contributor

bors commented Apr 28, 2022

☔ The latest upstream changes (presumably #91557) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 30, 2022

☔ The latest upstream changes (presumably #93803) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 7, 2022
Make lowering a query

Split from rust-lang#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 new `LocalDefId`s from inside it. This is done by:
- enclosing `Definitions` in a lock, so as to allow modification;
- creating a query `register_def` whose purpose is to declare a `LocalDefId` to the query system.

See `TyCtxt::create_def` and `TyCtxt::iter_local_def_id` for more detailed explanations of the design.
@JohnCSimon
Copy link
Member

@cjgillot
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.