Decouple rustc_resolve and rustc_ast_lowering by cjgillot · Pull Request #90451 · rust-lang/rust

cjgillot · 2021-10-31T18:44:13Z

~~Based on #89090 and #90446.~~
Part of #88186.

This PR removes the ResolverAstLowering trait which existed to pass information from resolution to lowering. The required information is passed through the ResolverOutputs struct.

r? @michaelwoerister

pierwill · 2021-10-31T21:39:37Z

Do they necessarily have resolutions in multiple namespaces?
Suggested change
/// Resolutions for import nodes, which have multiple resolutions in different namespaces.
/// Resolutions for import nodes, which can have multiple resolutions in different namespaces.

michaelwoerister · 2021-11-02T11:21:20Z

I think @petrochenkov might be more qualified to review this.

michaelwoerister · 2021-11-02T11:22:11Z

r? @petrochenkov (let me know if you don't want to review)

petrochenkov · 2021-11-04T18:19:53Z

Blocked on #89090 and #90446.

rust-timer · 2021-12-18T18:50:12Z

Queued 11af4eda005db3083b170328e09db3f9625d6adf with parent d3f3004, future comparison URL.

rust-timer · 2021-12-18T20:56:06Z

Finished benchmarking commit (11af4eda005db3083b170328e09db3f9625d6adf): comparison url.

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

Small improvement in instruction counts (up to -0.4% on incr-patched: println builds of coercions)
Small regression in instruction counts (up to 0.5% on incr-full builds of deeply-nested-async)

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-12-20T11:10:00Z

r=me on 5c4f828, f19a7f1 and baef1d2.

I need more context for other changes, why are they useful?
As for 0a78293, definitions and crate store are resolver outputs at least conceptually, what are the reasons to split them up?
For 63e9a5a I just need some more detailed design to understand the motivation.

cjgillot · 2021-12-27T17:59:23Z

The context is #88186: the objective is to make lowering a query. For this to happen, we need:

a way to create new LocalDefIds inside the query system, so Definitions needs special treatment;
CrateStore is used in a similar way to Definitions, so it just made sense to treat them so;
keep immutable resolver information while TyCtxt lives instead of directly using the Resolver (63e9a5a).

I agree these two commits do not bring any obvious value right now. I can delay them to another PR if you prefer.

petrochenkov · 2021-12-29T07:26:03Z

 /// `rustc_middle` is independent of the parser, we use dynamic dispatch here.
 nt_to_tokenstream: NtToTokenstream,

+ item_generics_num_lifetimes: fn(&Session, &CrateStoreDyn, DefId) -> usize,


Maybe it's better to make this a method on the CrateStore trait?

petrochenkov · 2021-12-29T07:26:31Z

- fn legacy_const_generic_args(&mut self, expr: &Expr) -> Option<Vec<usize>>;
-
- /// Obtains resolution for a `NodeId` with a single resolution.
+trait ResolverAstLoweringExt {


Why does this need to be a trait rather than a set of inherent methods?

petrochenkov · 2021-12-29T07:41:42Z

@cjgillot
I'd still like to know more details about #90451 (comment) (without reading the whole diff of #88186).

The main question is how mutable are ResolverOutputs, Definitions and CrateStore, and how long they live.

I expected ResolverOutputs to be immutable, but they now include things like next_node_id that are mutable.
I thought Definitions are separated from ResolverOutputs due to their mutability (they are mutable because new LocalDefIds need to be created), but that's not true given the previous sentence, ResolverOutputs are mutable too.
It's still not clear how mutable CrateStore is and why it's separated.

It also doesn't feel right that a bunch of tables in ResolverOutputs are only used during HIR lowering, but live forever now (as long as tcx).

Perhaps we need more separate structures created using the next criteria?
"Needed only during lowering vs Needed in tcx after lowering" x "Immutable vs Mutable only during lowering vs Mutable in tcx after lowering"

cjgillot · 2021-12-30T17:10:14Z

The basic idea is to make lowering incremental, so we will have to keep some resolver outputs longer than we currently have to.

The issue you have with mutable vs immutable outputs probably comes from a poor choice of mine when splitting up #88186. If you don't mind, I will close this PR and try to make the splitting more logical.

Definitions is considered as an interner, so will be mutable, behind a private RwLock.
ResolverOutputs and CrateStore will be immutable. next_node_id will be the highest NodeId given by the resolver, and lowering will manage subsequent NodeIds. trait_map will not be modified, HIR will just keep a reference to it.

Make lowering pull-based ~Based on rust-lang#90451 Part of rust-lang#88186 The current lowering code visits all the item-likes in the AST in order, and lowers them one by one. This PR changes it to index the AST and then proceed to lowering on-demand. This is closer to the logic of query-based lowering.

rust-highfive assigned michaelwoerister Oct 31, 2021

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