Lower only one HIR owner at a time by cjgillot · Pull Request #87234 · rust-lang/rust

cjgillot · 2021-07-17T23:52:00Z

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 · 2021-07-17T23:52:03Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2021-07-17T23:52:04Z

r? @davidtwco

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

jyn514 · 2021-07-17T23:55:31Z

cc @petrochenkov since this moves some things to rustc_resolve

petrochenkov · 2021-07-18T10:48:31Z

Blocked on #84373.

petrochenkov · 2021-07-20T11:38:01Z

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 · 2021-07-22T11:10:43Z

@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?

cjgillot · 2021-07-24T12:56:04Z

@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.

cjgillot · 2021-09-19T10:28:25Z

@bors try @rust-timer queue

rust-timer · 2021-09-19T10:28:27Z

Awaiting bors try build completion.

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

bors · 2021-09-19T10:28:34Z

⌛ Trying commit 95c7e884872435d0986dfde25f5925e2c0ad88bd with merge 9b09421d6a741b71ea5a76c626df819dda3cc5ed...

bors · 2021-09-19T11:47:21Z

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

rust-timer · 2021-09-19T11:47:22Z

Queued 9b09421d6a741b71ea5a76c626df819dda3cc5ed with parent 3bca723, future comparison URL.

Do not preallocate HirIds Part of rust-lang#87234 r? `@petrochenkov`

rust-timer · 2021-09-19T14:18:14Z

Finished benchmarking commit (9b09421d6a741b71ea5a76c626df819dda3cc5ed): comparison url.

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

Moderate improvement in instruction counts (up to -1.7% on full builds of unused-warnings)
Moderate regression in instruction counts (up to 0.9% on incr-patched: add static arr item builds of coercions)

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 @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

petrochenkov · 2021-09-19T22:13:51Z

@cjgillot
#88627 and #88703 has landed, could you rebase this?

petrochenkov · 2021-09-20T14:48:43Z

compiler/rustc_ast_lowering/src/item.rs

    fn with_parent_item_lifetime_defs<T>(
        &mut self,
-        parent_hir_id: hir::ItemId,
+        parent_hir_id: LocalDefId,


Suggested change

parent_hir_id: LocalDefId,

parent_def_id: LocalDefId,

petrochenkov · 2021-09-20T14:49:29Z

compiler/rustc_ast_lowering/src/item.rs

                // 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);


Suggested change

let new_hir_id = self.resolver.local_def_id(id);

let new_def_id = self.resolver.local_def_id(id);

petrochenkov · 2021-09-20T14:51:26Z

@bors r+

bors · 2021-09-20T14:51:28Z

📌 Commit 11024b2 has been approved by petrochenkov

bors · 2021-09-20T20:19:03Z

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

bors · 2021-09-21T00:34:04Z

💥 Test timed out

rust-log-analyzer · 2021-09-21T00:37:26Z

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

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

cjgillot · 2021-09-21T06:23:23Z

@bors retry

bors · 2021-09-21T07:52:22Z

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

bors · 2021-09-21T10:33:08Z

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

pnkfelix · 2021-09-21T21:42:58Z

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

rust-highfive assigned davidtwco Jul 17, 2021

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

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Jul 18, 2021

petrochenkov added S-blocked Status: 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

This comment has been minimized.

Sign in to view

petrochenkov mentioned this pull request Jul 20, 2021

Store all HIR owners in the same container #83723

Merged

davidtwco removed their assignment Jul 20, 2021

cjgillot force-pushed the lower-mono branch from 22af59a to 5be304e Compare July 21, 2021 18:58

This comment has been minimized.

Sign in to view

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: Blocked on something else such as an RFC or other implementation work. labels Jul 23, 2021

cjgillot mentioned this pull request Jul 23, 2021

Macro definitions should be regular HIR items #87406

Closed

cjgillot force-pushed the lower-mono branch from 5be304e to b665c89 Compare July 24, 2021 09:47

This comment has been minimized.

Sign in to view

cjgillot force-pushed the lower-mono branch 2 times, most recently from 3514603 to 5f067ce Compare July 24, 2021 12:55

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 19, 2021

bors mentioned this pull request Sep 19, 2021

Cleanup: Remove needless reference in ParentHirIterator #89078

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2021

Auto merge of rust-lang#88627 - cjgillot:noallocuse, r=petrochenkov

697118d

Do not preallocate HirIds Part of rust-lang#87234 r? `@petrochenkov`

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 19, 2021

cjgillot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 19, 2021

cjgillot added 4 commits September 20, 2021 00:29

Do not store visibility in *ItemRef.

d7795d3

Remove lower_node_id_with_owner.

c1bac92

Make with_hir_id_owner responsible for registering the item.

a1a3557

Bless incremental tests.

11024b2

cjgillot force-pushed the lower-mono branch from 95c7e88 to 11024b2 Compare September 20, 2021 06:50

petrochenkov reviewed Sep 20, 2021

View reviewed changes

bors mentioned this pull request Sep 21, 2021

Encode spans relative to the enclosing item -- enable on nightly #84762

Merged

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

Uh oh!

Conversation

cjgillot commented Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jul 17, 2021

Uh oh!

rust-highfive commented Jul 17, 2021

Uh oh!

jyn514 commented Jul 17, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Jul 18, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

petrochenkov commented Jul 22, 2021

Uh oh!

This comment has been minimized.

cjgillot commented Jul 24, 2021

Uh oh!

cjgillot commented Sep 19, 2021

Uh oh!

rust-timer commented Sep 19, 2021

Uh oh!

bors commented Sep 19, 2021

Uh oh!

bors commented Sep 19, 2021

Uh oh!

rust-timer commented Sep 19, 2021

Uh oh!

rust-timer commented Sep 19, 2021

Uh oh!

petrochenkov commented Sep 19, 2021

Uh oh!

petrochenkov Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

petrochenkov Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Sep 20, 2021

Uh oh!

bors commented Sep 20, 2021

Uh oh!

bors commented Sep 20, 2021

Uh oh!

bors commented Sep 21, 2021

Uh oh!

rust-log-analyzer commented Sep 21, 2021

Uh oh!

cjgillot commented Sep 21, 2021

Uh oh!

bors commented Sep 21, 2021

Uh oh!

bors commented Sep 21, 2021

Uh oh!

pnkfelix commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

cjgillot commented Jul 17, 2021 •

edited

Loading

petrochenkov commented Jul 20, 2021 •

edited

Loading