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.

Sign up for GitHub

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

Jump to bottom

Lower only one HIR owner at a time #87234

Merged
merged 4 commits into from
Sep 21, 2021
Merged

Lower only one HIR owner at a time #87234

merged 4 commits into from
Sep 21, 2021

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jul 17, 2021 •
edited
Loading

Based on #83723
Additional diff is here: cjgillot/rust@ownernode...lower-mono

Lowering is very tangled and has a tendency to intertwine the transformation of different items. This PR aims at simplifying the logic by:

  • moving global analyses to the resolver (item_generics_num_lifetimes, proc_macros, trait_impls);
  • removing a few special cases (non-exported macros and use statements);
  • restricting the amount of available information at any one time;
  • avoiding back-and-forth between different owners: an item must now be lowered all at once, and its parent cannot refer to its nodes.

I also removed the sorting of bodies by span. The diagnostic ordering changes marginally, since definitions are pretty much sorted already according to the AST. This uncovered a subtlety in thir-unsafeck.

(While these items could logically be in different PRs, the dependency between commits and the amount of conflicts force a monolithic PR.)

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 17, 2021

cc @petrochenkov since this moves some things to rustc_resolve

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov self-assigned this Jul 18, 2021
@petrochenkov
Copy link
Contributor

Blocked on #84373.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2021
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 20, 2021 •
edited
Loading

Wait a second, this PR is not actually based on #84373 like the description says.
Is it ready for review?

UPD: Looks like you've meant #83723.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 20, 2021
@petrochenkov petrochenkov mentioned this pull request Jul 20, 2021
Merged
@davidtwco davidtwco removed their assignment Jul 20, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

@cjgillot
Could you explain what's going on with the "HirId preallocation" changes in more details?
Why were the HirIds pre-allocated? Why don't they need to be pre-allocated now? When they are allocated if they are not pre-allocated? (What does it even mean to allocate a HirId?)
Why is item_local_id_counters keyed on NodeIds and not LocalDefIds despite the NodeIds having the owner semantics?

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 23, 2021
@cjgillot cjgillot mentioned this pull request Jul 23, 2021
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the lower-mono branch 2 times, most recently from 3514603 to 5f067ce Compare July 24, 2021 12:55
@cjgillot
Copy link
Contributor Author

@cjgillot
Could you explain what's going on with the "HirId preallocation" changes in more details?
Why were the HirIds pre-allocated? Why don't they need to be pre-allocated now? When they are allocated if they are not pre-allocated? (What does it even mean to allocate a HirId?)

By "HirId preallocation", for lack of a better word, I mean calls to allocate_hir_id_counter.
This pre-allocation basically declared to the lowering context that such NodeId was a HIR owner, and had to be lowered as such.
For item-likes, this was not necessary: we know that the nodes are only lowered when lowering the item itself, or when lowering a reference to the item. Calling allocate_hir_id_counter at that moment is enough.
For use items, the situation is essentially the same, with the additional subtlety that each use is associated with several definitions, and only part of those definitions actually get referenced in the HIR.

Why is item_local_id_counters keyed on NodeIds and not LocalDefIds despite the NodeIds having the owner semantics?

I had changed that before messing up the rebase.

@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 24, 2021
petrochenkov
petrochenkov reviewed Sep 20, 2021
View reviewed changes
compiler/rustc_ast_lowering/src/item.rs
@@ -106,12 +94,12 @@ impl<'hir> LoweringContext<'_, 'hir> {
// only used when lowering a child item of a trait or impl.
fn with_parent_item_lifetime_defs<T>(
&mut self,
parent_hir_id: hir::ItemId,
parent_hir_id: LocalDefId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parent_hir_id: LocalDefId,
parent_def_id: LocalDefId,

compiler/rustc_ast_lowering/src/item.rs
@@ -550,7 +546,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

// Add all the nested `PathListItem`s to the HIR.
for &(ref use_tree, id) in trees {
let new_hir_id = self.allocate_hir_id_counter(id);
let new_hir_id = self.resolver.local_def_id(id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let new_hir_id = self.resolver.local_def_id(id);
let new_def_id = self.resolver.local_def_id(id);

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 20, 2021

📌 Commit 11024b2 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2021
@bors
Copy link
Contributor

bors commented Sep 20, 2021

⌛ Testing commit 11024b2 with merge 7da30f2d33a61ad8b1742097dd71ffc698d1dc8c...

@bors
Copy link
Contributor

bors commented Sep 21, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 21, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@cjgillot
Copy link
Contributor Author

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2021
@bors
Copy link
Contributor

bors commented Sep 21, 2021

⌛ Testing commit 11024b2 with merge 49c0861...

@bors
Copy link
Contributor

bors commented Sep 21, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 49c0861 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 21, 2021
@bors bors merged commit 49c0861 into rust-lang:master Sep 21, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 21, 2021
@bors bors mentioned this pull request Sep 21, 2021
Merged
@cjgillot cjgillot deleted the lower-mono branch September 21, 2021 15:12
@pnkfelix
Copy link
Member

I think this commit was one that was left out of the normal perf service.

Here is what the weekly perf triage report says about it:

Lower only one HIR owner at a time #87234

  • Large improvement in instruction counts (up to -1.3% on full builds of unused-warnings)
  • Small regression in instruction counts (up to 0.4% on incr-unchanged builds of helloworld)

Looking at the results by eye, I'd say its clearly a big win.

@rustbot label: perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.