Remove unused `Lift` impls #115831

nnethercote · 2023-09-14T02:47:53Z

r? @bjorn3

rustbot · 2023-09-14T02:48:03Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in abstract_const.rs

cc @BoxyUwU

nnethercote · 2023-09-14T02:48:16Z

@bors try @rust-timer queue

bors · 2023-09-14T02:48:26Z

⌛ Trying commit db8fe2b with merge 0b9b838...

…, r=<try> Remove unused `Lift` impls r? `@bjorn3`

bors · 2023-09-14T03:58:48Z

☀️ Try build successful - checks-actions
Build commit: 0b9b838 (0b9b8384fd0807db47cf71475bd6a56fa7fe07ce)

bjorn3 · 2023-09-14T09:59:14Z

compiler/rustc_middle/src/ty/instance.rs

@@ -19,14 +19,14 @@ use std::fmt;
 /// simply couples a potentially generic `InstanceDef` with some args, and codegen and const eval
 /// will do all required substitution as they run.
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, TyEncodable, TyDecodable)]
-#[derive(HashStable, Lift, TypeFoldable, TypeVisitable)]
+#[derive(HashStable, TypeFoldable, TypeVisitable)]


This one is used by cg_clif's jit mode: https://github.com/bjorn3/rustc_codegen_cranelift/blob/d72f7109ede688200c4ffdc0453457a66552f47b/src/driver/jit.rs#L256

I have reinstated it, along with the ones for InstanceDef and Option, which were also required.

rustc_codegen_cranelift's Cargo.toml says the jit feature is "not ready to be enabled when compiling as part of rustc", which is a bit annoying when making this kind of change. Is that still true? If so, what is required to enable it?

You can edit Cargo.toml to add jit to the default features for testing. It is indeed still not ready to be enabled by default. std::env::args() is broken on windows, macos and linux with glibc. (on these targets the argv passed to the main function is ignored), the jitted code doesn't run on the main thread which breaks everything using winit and in general it is slower due to incr comp being broken as an exit from the jitted main function will immediately exit the rustc process and because cranelift-jit doesn't support serializing to a cachable object file. It also requires all dependencies to be available as dylib.

Would it be possible to add jit to the features within rustc just so it gets compiled, even if it's not tested/run in any way? That would be enough to avoid the above problem.

Should be possible to add it to check builds.

Cool. Doesn't need to be in this PR, though, which I think is ready to be merged.

nnethercote · 2023-09-14T22:32:39Z

I have updated to reinstate the impl for Instance and dependent impls.

rust-timer · 2023-09-15T00:20:59Z

Finished benchmarking commit (0b9b838): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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)	-	-	0
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-1.7%	[-1.7%, -1.7%]	1

Cycles

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

Binary size

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

Bootstrap: 631.556s -> 629.187s (-0.38%)
Artifact size: 317.95 MiB -> 317.86 MiB (-0.03%)

nnethercote · 2023-09-15T03:53:33Z

It's a 90 KiB artifact size reduction, which is nice.

bjorn3 · 2023-09-15T07:18:27Z

compiler/rustc_middle/src/ty/print/pretty.rs

    ty::Binder<'tcx, ty::ProjectionPredicate<'tcx>>,
-    ty::Binder<'tcx, ty::OutlivesPredicate<Ty<'tcx>, ty::Region<'tcx>>>,
-    ty::Binder<'tcx, ty::OutlivesPredicate<ty::Region<'tcx>, ty::Region<'tcx>>>,


Not sure if we should do this commit. It may be useful for debugging out-of-tree tools.

Well, let me know when you're sure one way or the other.

I think we should keep it. r=me with this commit dropped.

Just to clarify, you want the entire "Remove unused Display impls." commit removed? That will likely have knock-on effects, reducing the number of Lift impls that can be removed in the later commits.

bjorn3 · 2023-09-16T10:04:28Z

@bors r+

bors · 2023-09-16T10:04:29Z

📌 Commit 2e7c41e has been approved by bjorn3

It is now in the queue for this repository.

bors · 2023-09-16T13:10:04Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout remove-unused-Lift-impls (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self remove-unused-Lift-impls --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging compiler/rustc_middle/src/ty/sty.rs
Auto-merging compiler/rustc_middle/src/ty/structural_impls.rs
CONFLICT (content): Merge conflict in compiler/rustc_middle/src/ty/structural_impls.rs
Auto-merging compiler/rustc_middle/src/ty/instance.rs
Auto-merging compiler/rustc_middle/src/ty/generic_args.rs
Auto-merging compiler/rustc_middle/src/ty/context.rs
Auto-merging compiler/rustc_middle/src/ty/consts/kind.rs
Auto-merging compiler/rustc_middle/src/mir/mod.rs
Auto-merging compiler/rustc_middle/src/mir/interpret/mod.rs
Auto-merging compiler/rustc_middle/src/mir/interpret/error.rs
Automatic merge failed; fix conflicts and then commit the result.

It's easier to pass it in to the one method that needs it (`highlighting_region_vid`) than to store it in the type. This means `RegionHighlightMode` can impl `Default`.

To match `TrivialTypeTraversalImpls` and `TrivialTypeTraversalAndLiftImpls`, and because the `Clone` doesn't mean anything.

I found these by commenting out all `Lift` derives and then adding back the ones that were necessary to successfully compile.

nnethercote · 2023-09-18T00:25:00Z

I rebased. It was one of those mysterious "conflicts" that didn't actually involve any conflicts when I rebased locally. Hmm.

@bors r=bjorn3

bors · 2023-09-18T00:25:02Z

📌 Commit abe2a68 has been approved by bjorn3

It is now in the queue for this repository.

bors · 2023-09-18T05:20:18Z

⌛ Testing commit abe2a68 with merge 24bb884...

bors · 2023-09-18T07:07:21Z

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 24bb884 to master...

rust-timer · 2023-09-18T08:29:26Z

Finished benchmarking commit (24bb884): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.5%]	4
All ❌✅ (primary)	-	-	0

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)	-	-	0
Improvements ✅ (primary)	-1.9%	[-1.9%, -1.9%]	1
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	-1.9%	[-1.9%, -1.9%]	1

Cycles

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

Binary size

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

Bootstrap: 634.721s -> 633.229s (-0.24%)
Artifact size: 318.13 MiB -> 317.99 MiB (-0.05%)

rustbot assigned bjorn3 Sep 14, 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. labels Sep 14, 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 Sep 14, 2023

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

Auto merge of rust-lang#115831 - nnethercote:remove-unused-Lift-impls…

0b9b838

…, r=<try> Remove unused `Lift` impls r? `@bjorn3`

This comment has been minimized.

Sign in to view

bjorn3 reviewed Sep 14, 2023

View reviewed changes

nnethercote force-pushed the remove-unused-Lift-impls branch from db8fe2b to 2e7c41e Compare September 14, 2023 22:30

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

bjorn3 reviewed Sep 15, 2023

View reviewed changes

nnethercote mentioned this pull request Sep 16, 2023

make ty::Const debug printing less verbose #115884

Merged

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 16, 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 Sep 16, 2023

nnethercote added 4 commits September 18, 2023 09:37

Remove RegionHighlightMode::tcx.

1281d43

It's easier to pass it in to the one method that needs it (`highlighting_region_vid`) than to store it in the type. This means `RegionHighlightMode` can impl `Default`.

Remove unused Display impls.

46fe65d

Rename CloneLiftImpls as TrivialLiftImpls.

6b1980f

To match `TrivialTypeTraversalImpls` and `TrivialTypeTraversalAndLiftImpls`, and because the `Clone` doesn't mean anything.

Remove unused Lift derives.

af7d3e5

I found these by commenting out all `Lift` derives and then adding back the ones that were necessary to successfully compile.

Remove more unused Lift impls.

abe2a68

nnethercote force-pushed the remove-unused-Lift-impls branch from 2e7c41e to abe2a68 Compare September 18, 2023 00:19

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 Sep 18, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 18, 2023

bors merged commit 24bb884 into rust-lang:master Sep 18, 2023

rustbot added this to the 1.74.0 milestone Sep 18, 2023

nnethercote deleted the remove-unused-Lift-impls branch September 18, 2023 07:41

bors mentioned this pull request Sep 18, 2023

move things out of mir/mod.rs #115865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused `Lift` impls #115831

Remove unused `Lift` impls #115831

nnethercote commented Sep 14, 2023

rustbot commented Sep 14, 2023

nnethercote commented Sep 14, 2023

This comment has been minimized.

bors commented Sep 14, 2023

bors commented Sep 14, 2023

This comment has been minimized.

bjorn3 Sep 14, 2023

nnethercote Sep 14, 2023

bjorn3 Sep 14, 2023

nnethercote Sep 14, 2023

bjorn3 Sep 15, 2023

nnethercote Sep 15, 2023

nnethercote commented Sep 14, 2023

rust-timer commented Sep 15, 2023

nnethercote commented Sep 15, 2023

bjorn3 Sep 15, 2023

nnethercote Sep 15, 2023

bjorn3 Sep 15, 2023

nnethercote Sep 15, 2023

bjorn3 Sep 16, 2023

bjorn3 commented Sep 16, 2023

bors commented Sep 16, 2023

bors commented Sep 16, 2023

nnethercote commented Sep 18, 2023

bors commented Sep 18, 2023

bors commented Sep 18, 2023

bors commented Sep 18, 2023

rust-timer commented Sep 18, 2023

Remove unused Lift impls #115831

Remove unused Lift impls #115831

Conversation

nnethercote commented Sep 14, 2023

rustbot commented Sep 14, 2023

nnethercote commented Sep 14, 2023

This comment has been minimized.

bors commented Sep 14, 2023

bors commented Sep 14, 2023

This comment has been minimized.

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

nnethercote commented Sep 14, 2023

rust-timer commented Sep 15, 2023

Overall result: no relevant changes - no action needed

nnethercote commented Sep 15, 2023

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

bjorn3 commented Sep 16, 2023

bors commented Sep 16, 2023

bors commented Sep 16, 2023

nnethercote commented Sep 18, 2023

bors commented Sep 18, 2023

bors commented Sep 18, 2023

bors commented Sep 18, 2023

rust-timer commented Sep 18, 2023

Overall result: ✅ improvements - no action needed

Remove unused `Lift` impls #115831

Remove unused `Lift` impls #115831