Remove movability from `TyKind::Coroutine` #119174

compiler-errors · 2023-12-21T01:53:40Z

There's no reason to store movability in the generator struct directly. It is computed from the HIR, and can be pulled into a query to access when necessary.

rustbot · 2023-12-21T01:53:45Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Type relation code was changed

cc @compiler-errors, @lcnr

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

compiler-errors · 2023-12-21T01:54:34Z

lol i guess this could've been a draft -- forgot that I touched so many files 💀

@bors try @rust-timer queue

bors · 2023-12-21T01:55:43Z

⌛ Trying commit 3b7be08 with merge 23cf56b...

Remove movability from `TyKind::Coroutine` This seems like something that we can just have a query to compute... let's see what this does to perf. r? `@ghost`

bors · 2023-12-21T03:24:45Z

☀️ Try build successful - checks-actions
Build commit: 23cf56b (23cf56bcae642b8503b32c236002e65de6eecb25)

rust-timer · 2023-12-21T07:14:19Z

Finished benchmarking commit (23cf56b): 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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
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)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

Binary size

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

Bootstrap: 672.809s -> 671.782s (-0.15%)
Artifact size: 312.78 MiB -> 312.78 MiB (0.00%)

compiler-errors · 2023-12-21T15:54:19Z

r? compiler

nnethercote · 2023-12-21T20:26:38Z

compiler/rustc_codegen_cranelift/src/value_and_place.rs

-        (&ty::Coroutine(def_id_a, args_a, mov_a), &ty::Coroutine(def_id_b, args_b, mov_b))
-            if def_id_a == def_id_b && mov_a == mov_b =>
+        (&ty::Coroutine(def_id_a, args_a), &ty::Coroutine(def_id_b, args_b))
+            if def_id_a == def_id_b =>


Is this safe?

Yes. Movability is a property uniquely determined by def-id -- its inclusion here was redundant.

celinval · 2023-12-21T20:34:27Z

compiler/stable_mir/src/ty.rs

@@ -460,7 +460,7 @@ pub enum RigidTy {
    FnDef(FnDef, GenericArgs),
    FnPtr(PolyFnSig),
    Closure(ClosureDef, GenericArgs),
-    Coroutine(CoroutineDef, GenericArgs, Movability),


I was wondering if we could leave this here for now and make it always the same value. We could add a comment documenting that for now?

BTW, we need to figure out the best mechanism to make breaking changes to StableMIR, but this one I believe we can delay this one for now.

What do you mean by making it always the same value?

I'd rather expose a fn movability(CoroutineDef) -> Movability fn in the long term, but I can "pre-populate" it here I guess for the mean time.

Yes, that would work. I misunderstood the reason why you were removing it.

Which would you like for me to do first? Make smir::TyKind::Coroutine keep the Movability argument even though it's redudant, or add a fn movability(CoroutineDef) -> Movability to the tables?

I've just restored movability to SMIR, and added a FIXME(stable_mir).

bors · 2023-12-22T07:02:52Z

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

compiler/rustc_middle/src/query/mod.rs

bors · 2023-12-26T07:19:29Z

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

cjgillot

A few nits and r=me

compiler/rustc_metadata/src/rmeta/table.rs

compiler/rustc_middle/src/ty/context.rs

compiler/rustc_middle/src/ty/parameterized.rs

compiler-errors · 2023-12-28T17:35:22Z

@bors r=cjgillot

bors · 2023-12-28T17:35:25Z

📌 Commit e24da8e has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-12-28T20:41:47Z

⌛ Testing commit e24da8e with merge fb5ed72...

bors · 2023-12-28T22:51:29Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing fb5ed72 to master...

rust-timer · 2023-12-29T00:46:29Z

Finished benchmarking commit (fb5ed72): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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)	4.7%	[4.2%, 5.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	2

Binary size

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

Bootstrap: 672.476s -> 670.748s (-0.26%)
Artifact size: 312.31 MiB -> 312.31 MiB (-0.00%)

Relevant PRs: - rust-lang/rust#119601 - rust-lang/rust#119146 - rust-lang/rust#119174 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Dec 21, 2023

rust-cloud-vms bot force-pushed the movability branch from f6109ae to 3b7be08 Compare December 21, 2023 01:54

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 Dec 21, 2023

compiler-errors marked this pull request as draft December 21, 2023 01:54

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 Dec 21, 2023

rust-cloud-vms bot force-pushed the movability branch from 3b7be08 to a74f6d9 Compare December 21, 2023 15:54

compiler-errors marked this pull request as ready for review December 21, 2023 15:54

rustbot assigned davidtwco Dec 21, 2023

nnethercote reviewed Dec 21, 2023

View reviewed changes

celinval reviewed Dec 21, 2023

View reviewed changes

rust-cloud-vms bot force-pushed the movability branch from a74f6d9 to d991a97 Compare December 25, 2023 17:04

cjgillot reviewed Dec 25, 2023

View reviewed changes

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the movability branch 2 times, most recently from 3a9ec21 to 016694c Compare December 26, 2023 22:43

cjgillot reviewed Dec 27, 2023

View reviewed changes

compiler/rustc_metadata/src/rmeta/table.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/context.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/parameterized.rs Outdated Show resolved Hide resolved

petrochenkov mentioned this pull request Dec 28, 2023

aligns the behavior to that prior to #118311 #118411

Closed

compiler-errors added 3 commits December 28, 2023 16:35

Remove movability from TyKind::Coroutine

fcb42b4

Restore movability to SMIR

15ccf2e

Movability doesn't need to be a query anymore

e24da8e

rust-cloud-vms bot force-pushed the movability branch from 016694c to e24da8e Compare December 28, 2023 16:35

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 Dec 28, 2023

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

bors merged commit fb5ed72 into rust-lang:master Dec 28, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 28, 2023

This was referenced Dec 28, 2023

WIP: Remove ResumeTy from async lowering #107562

Draft

Couple of random coroutine pass simplifications #119322

Merged

Provide more context on recursive impl evaluation overflow #119389

Merged

compiler-errors deleted the movability branch December 29, 2023 05:40

zhassan-aws mentioned this pull request Jan 8, 2024

Upgrade to 2024-01-08 rust toolchain model-checking/kani#2969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove movability from `TyKind::Coroutine` #119174

Remove movability from `TyKind::Coroutine` #119174

compiler-errors commented Dec 21, 2023 •

edited

Loading

rustbot commented Dec 21, 2023

compiler-errors commented Dec 21, 2023

This comment has been minimized.

bors commented Dec 21, 2023

This comment has been minimized.

bors commented Dec 21, 2023

This comment has been minimized.

rust-timer commented Dec 21, 2023

compiler-errors commented Dec 21, 2023

nnethercote Dec 21, 2023

compiler-errors Dec 21, 2023

celinval Dec 21, 2023

compiler-errors Dec 21, 2023

celinval Dec 22, 2023

compiler-errors Dec 22, 2023

compiler-errors Dec 25, 2023

bors commented Dec 22, 2023

bors commented Dec 26, 2023

cjgillot left a comment

compiler-errors commented Dec 28, 2023

bors commented Dec 28, 2023

bors commented Dec 28, 2023

bors commented Dec 28, 2023

rust-timer commented Dec 29, 2023

Remove movability from TyKind::Coroutine #119174

Remove movability from TyKind::Coroutine #119174

Conversation

compiler-errors commented Dec 21, 2023 • edited Loading

rustbot commented Dec 21, 2023

compiler-errors commented Dec 21, 2023

This comment has been minimized.

bors commented Dec 21, 2023

This comment has been minimized.

bors commented Dec 21, 2023

This comment has been minimized.

rust-timer commented Dec 21, 2023

Overall result: ✅ improvements - no action needed

compiler-errors commented Dec 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 22, 2023

bors commented Dec 26, 2023

cjgillot left a comment

Choose a reason for hiding this comment

compiler-errors commented Dec 28, 2023

bors commented Dec 28, 2023

bors commented Dec 28, 2023

bors commented Dec 28, 2023

rust-timer commented Dec 29, 2023

Overall result: no relevant changes - no action needed

Remove movability from `TyKind::Coroutine` #119174

Remove movability from `TyKind::Coroutine` #119174

compiler-errors commented Dec 21, 2023 •

edited

Loading