Use non-ascribed type as field's type in mir #103880

b-naber · 2022-11-02T12:55:25Z

lcnr

thanks ❤️ a few nits

can you also add a test for infallible enum variants: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=62fcc4d83fb144317784e548798d86e9 and https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9cab784eee21f76c52fdbe0fb84f654d

and also enums with multiple variants: (not sure how that test should behave tbh) https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=74134c559bb8b169788b8ca5694674b3

compiler/rustc_mir_build/src/build/matches/util.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

lcnr · 2022-11-02T14:36:15Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

that's clever but I am not sure whether it is necessary 🤔

afaict the only reason why this returns Err is if the upvar is not part of the final mir body so it doesn't affect mir borrowck.

I would prefer always returning None in the Err case i think

This is necessary for the following scenario (compiling with --edition==2021:

fn test1() { let t = (String::from("Hello"), String::from("bla")); let c = || { let (t1, t2) = t; //~^ WARN unused variable: `t1` //~| WARN unused variable: `t2` }; c(); }

where y is not captured as an upvar and we only get an OpaqueCast projection in the PlaceBuilder.

I'm not sure what the correct behaviour here is. One could argue that the name of the method implies a best-effort approach, so that we get the current type of the PlaceBuilder whenever possible, in which case these additional projections would be necessary.

lcnr · 2022-11-02T14:39:04Z

compiler/rustc_mir_build/src/build/matches/util.rs

if my understanding is correct, this "if possible" is quite right, as getting the correct field type isn't relevant if try_ty returns None.

compiler/rustc_mir_build/src/build/matches/util.rs

bors · 2022-11-23T17:07:54Z

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

rustbot · 2022-11-23T20:26:02Z

This PR changes MIR

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

lcnr · 2022-11-24T09:27:06Z

@bors try @rust-timer queue

bors · 2022-11-24T09:27:15Z

⌛ Trying commit 9061ffb with merge 231fca09c3e445b08e289e6b8f254f38aa60052d...

bors · 2022-11-24T12:09:46Z

☀️ Try build successful - checks-actions
Build commit: 231fca09c3e445b08e289e6b8f254f38aa60052d (231fca09c3e445b08e289e6b8f254f38aa60052d)

RalfJung · 2022-11-26T09:55:50Z

compiler/rustc_middle/src/mir/syntax.rs

 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[derive(TyEncodable, TyDecodable, HashStable, TypeFoldable, TypeVisitable)]
-pub enum ProjectionElem<V, T> {
+pub enum ProjectionElem<V, T1, T2> {


At this point it might be worth documenting what these types mean? Their names are not exactly descriptive.

b-naber · 2022-11-26T21:17:11Z

Why has the perf run not finished yet?

oli-obk · 2022-11-28T10:04:16Z

@rust-timer build 231fca09c3e445b08e289e6b8f254f38aa60052d

bors · 2022-11-29T16:06:44Z

☀️ Try build successful - checks-actions
Build commit: 31b351e335795f9bb7ac7deac9b27e7010770610 (31b351e335795f9bb7ac7deac9b27e7010770610)

rust-timer · 2022-11-29T20:25:22Z

Finished benchmarking commit (31b351e335795f9bb7ac7deac9b27e7010770610): comparison URL.

Overall result: ❌ regressions - 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-review -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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

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)	7.1%	[6.8%, 7.4%]	2
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)	-1.8%	[-2.2%, -1.5%]	2
All ❌✅ (primary)	-	-	0

b-naber · 2022-11-30T10:50:04Z

Any thoughts on those perf results? Don't see any other options to improve perf and I think we would have a similar perf hit when "calculating" the place type for a field projection in other approaches. Getting the type while projecting seems to me the more reasonable approach compared to doing it during the simplify stage of mir building.

oli-obk · 2022-11-30T11:03:43Z

This perf regression seems to be somewhere in the noise of that test.

Afaict this PR is only blocked on trying out the hypothetical alternative designs (like re-using OpaqueCast). I don't know if @lcnr was going to do that or what the plan was.

lcnr

a bunch of nits, I think the current design is fine and we don't have to test the OpaqueCast approach.

I think the core of my nits about clone_project and fresh infer var in field could be changed by changing PlaceBuilder::project to take ProjectionElem<Local, (), Ty<'tcx>> and then recomputing the field type in there.

Instead of requiring Builder for base_place.to_place you could move

        cx.tcx.intern_place_elems(&builder.projection);
        Some(Place { local, projection })

into a separate function, maybe Place::new(tcx, local, projections) and only take a TyCtxt for project.

compiler/rustc_middle/src/mir/syntax.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

compiler/rustc_mir_build/src/build/matches/util.rs

compiler/rustc_mir_build/src/build/matches/test.rs

compiler/rustc_mir_build/src/build/expr/into.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

lcnr · 2022-12-16T11:07:39Z

@bors r+

bors · 2022-12-16T11:07:41Z

📌 Commit ff41359 has been approved by lcnr

It is now in the queue for this repository.

bors · 2022-12-16T12:47:52Z

⌛ Testing commit ff41359 with merge 03770f0...

bors · 2022-12-16T15:11:53Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 03770f0 to master...

rust-timer · 2022-12-16T17:45:50Z

Finished benchmarking commit (03770f0): comparison URL.

Overall result: ❌ regressions - 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)	3.1%	[2.5%, 3.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
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)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	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)	1.7%	[1.0%, 2.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

This reverts commit 03770f0, reversing changes made to 01ef4b2.

Revert rust-lang#103880 "Use non-ascribed type as field's type in mir" This PR prepares a revert for rust-lang#103880 to fix rust-lang#105809, rust-lang#105881, rust-lang#105886 and others (like the duplicates of the first one), in case an actual fix can't get done today. I've also added the MCVE from rust-lang#105809. There is no MCVE for the rust-lang#105881 and rust-lang#105886 ICEs yet however, so there are no tests for them here, although we'll need one before relanding the original changes. Were this PR to land, it would also reopen rust-lang#96514 as it was fixed by the original PR. Opening as draft to allow time for a possible fix. r? `@jackh726`

Use non-ascribed type as field's type in mir Fixes rust-lang#96514 r? `@lcnr`

rustbot assigned lcnr Nov 2, 2022

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 Nov 2, 2022

b-naber force-pushed the field-ty-mir branch from edf78c7 to 373c632 Compare November 2, 2022 13:01

lcnr reviewed Nov 2, 2022

View reviewed changes

b-naber force-pushed the field-ty-mir branch from 855f9de to 329a765 Compare November 3, 2022 11:13

b-naber added 10 commits November 23, 2022 19:58

implement PlaceBuilder::try_ty

ac31d52

use non-ascribed type as field type in mir

4d3a91b

add tests

906c527

properly handle enum field projections

f015842

add more tests

2ef8308

address review

fd6fed3

get field ty during projecting

4040734

add invariance test

4d00026

include closures and generators in try_compute_field_ty

c39de61

use no type in ProjectionElem::Field for PlaceBuilder::UpVar

9061ffb

b-naber force-pushed the field-ty-mir branch from 05c37ff to 9061ffb Compare November 23, 2022 20:25

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 Nov 24, 2022

RalfJung reviewed Nov 26, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

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 Nov 29, 2022

lcnr reviewed Dec 6, 2022

View reviewed changes

address review

ff41359

b-naber force-pushed the field-ty-mir branch from 13619e6 to ff41359 Compare December 14, 2022 20:32

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 16, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2022

bors merged commit 03770f0 into rust-lang:master Dec 16, 2022

rustbot added this to the 1.68.0 milestone Dec 16, 2022

b-naber deleted the field-ty-mir branch December 16, 2022 16:56

rustbot added the perf-regression Performance regression. label Dec 16, 2022

This was referenced Dec 17, 2022

ICE: Unexpected type Alias(...) for 'Field' projection , edition 2021 #105819

Closed

deref_target_place is not a deref projection #105881

Closed

albertlarsan68 mentioned this pull request Dec 19, 2022

Cbindgen fails on latest nightly (1.68.0-nightly) with an internal compiler error #105886

Closed

lqd added a commit to lqd/rust that referenced this pull request Dec 19, 2022

Revert "Auto merge of rust-lang#103880 - b-naber:field-ty-mir, r=lcnr"

8275d11

This reverts commit 03770f0, reversing changes made to 01ef4b2.

lqd mentioned this pull request Dec 19, 2022

Revert #103880 "Use non-ascribed type as field's type in mir" #105905

Merged

b-naber mentioned this pull request Dec 20, 2022

Fix for reverted PR #103880 #105958

Closed

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023

Auto merge of rust-lang#103880 - b-naber:field-ty-mir, r=lcnr

3b0f0b7

Use non-ascribed type as field's type in mir Fixes rust-lang#96514 r? `@lcnr`

Use non-ascribed type as field's type in mir #103880

Use non-ascribed type as field's type in mir #103880

Uh oh!

Conversation

b-naber commented Nov 2, 2022

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Nov 23, 2022

Uh oh!

rustbot commented Nov 23, 2022

Uh oh!

lcnr commented Nov 24, 2022

Uh oh!

This comment has been minimized.

bors commented Nov 24, 2022

Uh oh!

bors commented Nov 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-naber commented Nov 26, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

oli-obk commented Nov 28, 2022

Uh oh!

bors commented Nov 29, 2022

Uh oh!

This comment has been minimized.

rust-timer commented Nov 29, 2022

Overall result: ❌ regressions - no action needed

Uh oh!

b-naber commented Nov 30, 2022

Uh oh!

oli-obk commented Nov 30, 2022

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcnr commented Dec 16, 2022

Uh oh!

bors commented Dec 16, 2022

Uh oh!

bors commented Dec 16, 2022

Uh oh!

bors commented Dec 16, 2022

Uh oh!

rust-timer commented Dec 16, 2022

Overall result: ❌ regressions - ACTION NEEDED

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!