Fix for reverted PR #103880 #105958

b-naber · 2022-12-20T14:17:01Z

This re-introduces the changes in #103880, which was reverted in #105905.

The bug was caused by an unnecessary OpaqueCast, which was inserted because the condition for its introduction was violated by changing the field type.

Fixes #105881 (thanks to @trinity-1686a for the mcve)
Fixes #105819

#105905 already introduced a test for #105809

I successfully compiled cbindgen with this fix (#105886), but don't have a mcve for that issue.

r? @lcnr

rustbot · 2022-12-20T14:17:09Z

This PR changes MIR

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

Mark-Simulacrum · 2022-12-20T15:11:26Z

Let's make sure we run perf on this before merging and decide whether the regression seen on #103880 (comment) is justified.

b-naber · 2022-12-20T15:42:37Z

Let's make sure we run perf on this before merging and decide whether the regression seen on #103880 (comment) is justified.

Yes, also saw that. We did run perf twice on the previous PR though.

Can you start a perf run?

lqd · 2022-12-20T16:04:46Z

@bors try @rust-timer queue

bors · 2022-12-20T16:04:54Z

⌛ Trying commit 2bd63fc with merge 3be3d9020c6273426964e1caf1859956d1e6d2fd...

bors · 2022-12-20T18:33:08Z

☀️ Try build successful - checks-actions
Build commit: 3be3d9020c6273426964e1caf1859956d1e6d2fd (3be3d9020c6273426964e1caf1859956d1e6d2fd)

Mark-Simulacrum · 2022-12-20T19:03:54Z

Yes, also saw that. We did run perf twice on the previous PR though.

I didn't see -- perhaps I missed it -- a comment explaining the performance regression. That's something we hope/expect PR authors/reviewers to write up prior to merging a PR with known regressions.

rust-timer · 2022-12-20T19:52:42Z

Finished benchmarking commit (3be3d9020c6273426964e1caf1859956d1e6d2fd): comparison URL.

Overall result: ❌ regressions - 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.

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-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.7%	[0.3%, 1.2%]	5
Regressions ❌ (secondary)	2.5%	[2.0%, 4.1%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.3%, 1.2%]	5

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

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)	-2.9%	[-4.5%, -1.2%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-4.5%, -1.2%]	7

lcnr · 2022-12-21T11:51:04Z

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

@@ -109,7 +109,7 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> {
        let may_need_cast = match place {
            PlaceBuilder::Local { local, ref projection } => {
                let ty = Place::ty_from(local, projection, &cx.local_decls, cx.tcx).ty;
-                ty != pattern.ty && ty.has_opaque_types()
+                ty.has_opaque_types() && !pattern.ty.has_opaque_types()


i fear that this may only make the bug harder to trigger rather than actually fixing it.

You could imagine having a field like (subtype, opaque) to (supertype, opaque) which hits both the old and the new check.

What is the exact reason that we get an ICE there? Is the issue subtyping in OpaqueCast or something else?

hm I'm not sure, the comment above l.109 states Only add the OpaqueCast projection if the given place is an opaque type and the expected type from the pattern is not. I don't know how OpaqueCasts are handled later or why we need them (they're only used for opaque types, aren't they?). Would we really need an OpaqueCast only because of the subtype/suptype here?

I think the ICE is triggered because the type in the OpaqueCast is still generic in codegen and we therefore cannot get a layout for it.

ah (subtype, opaque) to (supertype, opaque) doesn't trigger this with your fix.

still worried about (subtype, opaque) to (supertype, concrete).

I think the ICE is triggered because the type in the OpaqueCast is still generic in codegen and we therefore cannot get a layout for it.

This doesn't make sense to me. OpaqueCast should never be generic during codegen. We should substitute the concrete generic arguments during codegen if the OpaqueCast projection still exist there. An alternative might be that we're missing a normalization for the opaque cast but that also seems weird 🤔

apiraino · 2023-01-17T17:17:54Z

I assume the last round of review is finished and the review flag can be switched. @b-naber feel free to request a review with @rustbot ready, thanks!

(by the way #105881 can probably be striked out since already fixed and closed)

@rustbot author

albertlarsan68 · 2023-01-18T09:33:41Z

Tests have moved from src/test to tests, please rebase on top of new master.
@rustbot label -A-bootstrap

Use covariance on type relations of field projection types if possible It's fine to use covariance here unless we're in a mutating context. Fixes rust-lang#96514 Supersedes rust-lang#105958 r? `@lcnr`

b-naber · 2023-02-20T22:19:28Z

Closed in favor of #107969

Use covariance on type relations of field projection types if possible It's fine to use covariance here unless we're in a mutating context. Fixes rust-lang/rust#96514 Supersedes rust-lang/rust#105958 r? `@lcnr`

b-naber added 4 commits December 20, 2022 09:09

use non-ascribed field ty in place field projection

b2656e7

don't include additional opaquecast

b26fea5

add tests

15fa511

cargo.lock

2bd63fc

rustbot assigned lcnr Dec 20, 2022

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

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 20, 2022

lcnr reviewed Dec 21, 2022

View reviewed changes

rustbot 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 17, 2023

rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023

b-naber mentioned this pull request Feb 12, 2023

Use covariance on type relations of field projection types if possible #107969

Merged

b-naber closed this Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for reverted PR #103880 #105958

Fix for reverted PR #103880 #105958

b-naber commented Dec 20, 2022

rustbot commented Dec 20, 2022

Mark-Simulacrum commented Dec 20, 2022

b-naber commented Dec 20, 2022

lqd commented Dec 20, 2022

This comment has been minimized.

bors commented Dec 20, 2022

bors commented Dec 20, 2022

This comment has been minimized.

Mark-Simulacrum commented Dec 20, 2022

rust-timer commented Dec 20, 2022

lcnr Dec 21, 2022 •

edited

Loading

b-naber Dec 21, 2022

lcnr Dec 21, 2022

apiraino commented Jan 17, 2023

albertlarsan68 commented Jan 18, 2023

b-naber commented Feb 20, 2023

Fix for reverted PR #103880 #105958

Fix for reverted PR #103880 #105958

Conversation

b-naber commented Dec 20, 2022

rustbot commented Dec 20, 2022

Mark-Simulacrum commented Dec 20, 2022

b-naber commented Dec 20, 2022

lqd commented Dec 20, 2022

This comment has been minimized.

bors commented Dec 20, 2022

bors commented Dec 20, 2022

This comment has been minimized.

Mark-Simulacrum commented Dec 20, 2022

rust-timer commented Dec 20, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

lcnr Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

b-naber Dec 21, 2022

Choose a reason for hiding this comment

lcnr Dec 21, 2022

Choose a reason for hiding this comment

apiraino commented Jan 17, 2023

albertlarsan68 commented Jan 18, 2023

b-naber commented Feb 20, 2023

lcnr Dec 21, 2022 •

edited

Loading