Remove HirId -> LocalDefId map from HIR. #107206

cjgillot · 2023-01-22T18:12:42Z

Having this map in HIR prevents the creating of new definitions after HIR has been built.
Thankfully, we do not need it.

Based on #103902

rustbot · 2023-01-22T18:12:48Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-01-22T18:12:51Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

cjgillot · 2023-01-22T18:20:49Z

@bors try @rust-timer queue

bors · 2023-01-22T18:20:59Z

⌛ Trying commit d67bc5ba7e09381171f0029586e4e5cae885132e with merge 2d52b6d10b72a07b2ac1932319805ec304591e33...

bors · 2023-01-22T21:03:55Z

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

bors · 2023-01-22T21:03:55Z

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

rust-timer · 2023-01-22T22:23:49Z

Finished benchmarking commit (2d52b6d10b72a07b2ac1932319805ec304591e33): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	47
Improvements ✅ (secondary)	-0.5%	[-0.9%, -0.2%]	34
All ❌✅ (primary)	-0.3%	[-0.6%, -0.2%]	47

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[1.1%, 7.9%]	4
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.5%	[1.0%, 2.0%]	10
Regressions ❌ (secondary)	3.0%	[1.2%, 4.6%]	18
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[1.0%, 2.0%]	10

WaffleLapkin

I'm not sure what the implications of using different IDs are, but generally looks good to me (especially the perf improvement 👀 ).

r=me after #103902 gets merged, this is rebased on top of master and associated type defaults are removed.

P.S. sorry for so many nits .-.

WaffleLapkin · 2023-01-23T11:52:39Z

compiler/rustc_hir_analysis/src/autoderef.rs

@@ -96,14 +96,14 @@ impl<'a, 'tcx> Autoderef<'a, 'tcx> {
    pub fn new(
        infcx: &'a InferCtxt<'tcx>,
        param_env: ty::ParamEnv<'tcx>,
-        body_id: hir::HirId,
+        body_def_id: LocalDefId,


Maybe use the same name as the field (body_id)?

WaffleLapkin · 2023-01-23T13:40:31Z

src/tools/clippy/clippy_lints/src/methods/suspicious_map.rs

        let closure = expr_or_init(cx, map_arg);
-        if let Some(def_id) = cx.tcx.hir().opt_local_def_id(closure.hir_id);
-        if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(def_id);
-        let closure_body = cx.tcx.hir().body(body_id);
+        if let hir::ExprKind::Closure(closure) = closure.kind;


Maybe merge this with the prev statement?

if let hir::ExprKind::Closure(closure) = expr_or_init(cx, map_arg).kind;

WaffleLapkin · 2023-01-23T13:55:41Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -2305,10 +2300,12 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                                predicate.to_opt_poly_trait_pred().unwrap(),
                            );
                            if impl_candidates.len() < 10 {
+                                let hir =


WaffleLapkin · 2023-01-23T13:57:56Z

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

-            sym::ItemContext,
-            self.describe_enclosure(obligation.cause.body_id).map(|s| s.to_owned()),
-        )];
+        let body_hir = self.tcx.hir().local_def_id_to_hir_id(obligation.cause.body_id);


body_hir_id?

WaffleLapkin · 2023-01-23T14:01:39Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -535,8 +535,8 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {

        // FIXME: Add check for trait bound that is already present, particularly `?Sized` so we
        //        don't suggest `T: Sized + ?Sized`.
-        let mut hir_id = body_id;
-        while let Some(node) = self.tcx.hir().find(hir_id) {
+        let mut body_id = body_id;


This can be removed in favor of mut body_id: LocalDefId in the param list.

WaffleLapkin · 2023-01-23T14:32:09Z

compiler/rustc_hir_typeck/src/method/suggest.rs

@@ -1593,7 +1590,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                else { return };

        let map = self.infcx.tcx.hir();
-        let body = map.body(rustc_hir::BodyId { hir_id: self.body_id });
+        let body_hir = self.tcx.hir().body_owned_by(self.body_id);


body_hir_id?

WaffleLapkin · 2023-01-23T14:37:24Z

compiler/rustc_infer/src/infer/error_reporting/mod.rs

-        // it's an actual definition. According to the comments (e.g. in
-        // rustc_hir_analysis/check/compare_impl_item.rs:compare_predicate_entailment) the latter
-        // is relied upon by some other code. This might (or might not) need cleanup.


Do the comments mentioned here need an update too?

WaffleLapkin · 2023-01-23T14:49:36Z

compiler/rustc_lint/src/nonstandard_style.rs

-                    if sig.header.abi != Abi::Rust && cx.sess().contains_name(attrs, sym::no_mangle)
+                    if sig.header.abi != Abi::Rust
+                        && cx.tcx.has_attr(id.to_def_id(), sym::no_mangle)


Is has_attr better than contains_name?

WaffleLapkin · 2023-01-23T14:58:26Z

compiler/rustc_privacy/src/lib.rs

-            .into_iter()
-            .filter_map(|hir_id| tcx.hir().opt_local_def_id(hir_id))
-            .collect(),
+        old_error_set_ancestry: old_error_set_ancestry,


Suggested change

old_error_set_ancestry: old_error_set_ancestry,

old_error_set_ancestry,

WaffleLapkin · 2023-01-23T15:00:34Z

compiler/rustc_save_analysis/src/lib.rs

@@ -3,6 +3,7 @@
 #![recursion_limit = "256"]
 #![allow(rustc::potential_query_instability)]
 #![feature(never_type)]
+#![feature(associated_type_defaults)]


Please do not use associated type defaults, r-a chokes on them, rust-lang/rust-analyzer#13693.

cjgillot · 2023-01-26T03:31:27Z

@bors r=WaffleLapkin

bors · 2023-01-26T03:31:29Z

📌 Commit b0fc662bed3d016b200868b80c1d0cfecbc55dcd has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-01-27T10:01:44Z

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

bors · 2023-01-28T01:45:17Z

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

cjgillot · 2023-01-28T10:43:18Z

@bors r=WaffleLapkin

bors · 2023-01-28T10:43:20Z

📌 Commit 1aab86e has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-01-28T16:11:36Z

⌛ Testing commit 1aab86e with merge d6f0642...

bors · 2023-01-28T18:40:06Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing d6f0642 to master...

rust-timer · 2023-01-28T19:59:28Z

Finished benchmarking commit (d6f0642): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	5
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	7
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.3%]	7
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.2%, -1.8%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

nnethercote · 2023-01-29T22:41:51Z

The (small) improvements outweigh the (small) regressions.

@rustbot label: +perf-regression-triaged

Upgrade the Rust toolchain to 'nightly-2023-02-01'  This change should address the failing nightly [rustc test jobs](https://github.com/servo/servo/actions/workflows/nightly-rust.yml) For reference, these are the [relevant](rust-lang/rust#107206) [PRs](rust-lang/rust#104170) in rustc that I could find. Signed-off-by: Mukilan Thiyagarajan <mukilanthiagarajan@gmail.com> ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [X] These changes do not require tests because there are existing unit tests for script_plugins that do pass.

Remove HirId -> LocalDefId map from HIR. Having this map in HIR prevents the creating of new definitions after HIR has been built. Thankfully, we do not need it. Based on rust-lang#103902

rustbot assigned WaffleLapkin Jan 22, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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. labels Jan 22, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2023

WaffleLapkin self-requested a review January 23, 2023 14:06

WaffleLapkin approved these changes Jan 23, 2023

View reviewed changes

WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2023

cjgillot force-pushed the no-h2l-map branch 2 times, most recently from ac30163 to b0fc662 Compare January 25, 2023 20:28

cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2023

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 Jan 26, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 28, 2023

cjgillot added 3 commits January 28, 2023 09:51

Take a LocalDefId in hir::Visitor::visit_fn.

3175d03

Remove HirId -> LocalDefId map from HIR.

15d6325

Adapt ui-fulldeps.

1aab86e

cjgillot force-pushed the no-h2l-map branch from 55575bc to 1aab86e Compare January 28, 2023 09:55

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2023

bors merged commit d6f0642 into rust-lang:master Jan 28, 2023

rustbot added this to the 1.69.0 milestone Jan 28, 2023

cjgillot deleted the no-h2l-map branch January 28, 2023 18:40

rustbot added the perf-regression Performance regression. label Jan 28, 2023

xFrednet mentioned this pull request Jan 28, 2023

Bump rust nightly -> 2023-03-09 rust-marker/marker#101

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 29, 2023

mukilan mentioned this pull request Feb 1, 2023

Upgrade the Rust toolchain to 'nightly-2023-02-01' servo/servo#29318

Merged

5 tasks

tedinski mentioned this pull request Feb 3, 2023

Update Rust toolchain to nightly-2023-02-03 model-checking/kani#2184

Closed

4 tasks

qinheping mentioned this pull request Mar 9, 2023

Upgrade Rust toolchain to nightly-2023-02-03 model-checking/kani#2291

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove HirId -> LocalDefId map from HIR. #107206

Remove HirId -> LocalDefId map from HIR. #107206

cjgillot commented Jan 22, 2023

rustbot commented Jan 22, 2023

rustbot commented Jan 22, 2023

cjgillot commented Jan 22, 2023

This comment has been minimized.

bors commented Jan 22, 2023

This comment has been minimized.

bors commented Jan 22, 2023

bors commented Jan 22, 2023

This comment has been minimized.

rust-timer commented Jan 22, 2023

WaffleLapkin left a comment •

edited

Loading

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

WaffleLapkin Jan 23, 2023

cjgillot commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 27, 2023

bors commented Jan 28, 2023

cjgillot commented Jan 28, 2023

bors commented Jan 28, 2023

bors commented Jan 28, 2023

bors commented Jan 28, 2023

rust-timer commented Jan 28, 2023

nnethercote commented Jan 29, 2023

	old_error_set_ancestry: old_error_set_ancestry,
	old_error_set_ancestry,

Remove HirId -> LocalDefId map from HIR. #107206

Remove HirId -> LocalDefId map from HIR. #107206

Conversation

cjgillot commented Jan 22, 2023

rustbot commented Jan 22, 2023

rustbot commented Jan 22, 2023

cjgillot commented Jan 22, 2023

This comment has been minimized.

bors commented Jan 22, 2023

This comment has been minimized.

bors commented Jan 22, 2023

bors commented Jan 22, 2023

This comment has been minimized.

rust-timer commented Jan 22, 2023

Overall result: ✅ improvements - no action needed

WaffleLapkin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 27, 2023

bors commented Jan 28, 2023

cjgillot commented Jan 28, 2023

bors commented Jan 28, 2023

bors commented Jan 28, 2023

bors commented Jan 28, 2023

rust-timer commented Jan 28, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Jan 29, 2023

WaffleLapkin left a comment •

edited

Loading