Pessimistically assume opaque types are !Freeze #113617

oli-obk · 2023-07-12T10:15:44Z

This PR fixes a cycle bug, and thus rely entirely on the body-analysis of the constant, instead of trying to look at the opaque type's hidden type. The main issue is that const fn and const items share their analysis, even though const fn don't care about the resulting qualifications, and only need them within the body to detect currently-illegal things like mutable references or interior mutability behind references. If we pessimistically assume opaque types have interior mutability, we can avoid the cycle errors, but are still respecting the feature gates and soundness.

As the tests I added show, we assume that opaque types have interior mutable data, even if the hidden type is a String. Note that we have a body-analysis, which shows that for None or () there can be no interior mutability, and thus appear to be looking behind an opaque type, when we are in fact looking at a constant's body.

r? lang

rustbot · 2023-07-12T10:15:53Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2023-07-12T10:15:57Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

oli-obk · 2023-07-12T10:17:21Z

compiler/rustc_mir_transform/src/lib.rs

-    let const_qualifs = tcx.mir_const_qualif(def);
+
+    let const_qualifs = match tcx.def_kind(def) {
+        DefKind::Fn | DefKind::AssocFn | DefKind::Closure
+            if tcx.constness(def) == hir::Constness::Const
+                || tcx.is_const_default_method(def.to_def_id()) =>
+        {
+            tcx.mir_const_qualif(def)
+        }
+        DefKind::AssocConst
+        | DefKind::Const
+        | DefKind::Static(_)
+        | DefKind::InlineConst
+        | DefKind::AnonConst => tcx.mir_const_qualif(def),
+        _ => ConstQualifs::default(),
+    };


This change does not affect any logic and just reduces the number of mir_const_qualif calls, and thus hopefully also reduces the cost of caching things

oli-obk · 2023-07-12T10:18:02Z

@bors try @rust-timer queue

bors · 2023-07-12T10:18:13Z

⌛ Trying commit b39f459 with merge edce3b8a2c0692991244661e01c4b20430e8c3ed...

bors · 2023-07-12T11:33:15Z

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

lukas-code · 2023-07-12T17:41:06Z

Note that this can break some programs that currently compile on stable, for example:

const fn foo() -> impl Sized {}

const _: () = {
    let x = foo();
    let _ = &x;
    core::mem::forget(x)
};

Is this considered acceptable breakage? I guess you do have to go pretty out of your way to write code like that.

rust-timer · 2023-07-13T00:51:57Z

Finished benchmarking commit (edce3b8a2c0692991244661e01c4b20430e8c3ed): 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.6%	[-0.7%, -0.6%]	6
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	3
All ❌✅ (primary)	-0.6%	[-0.7%, -0.6%]	6

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.8%	[-2.1%, -1.3%]	3
Improvements ✅ (secondary)	-2.6%	[-3.2%, -1.9%]	5
All ❌✅ (primary)	-1.8%	[-2.1%, -1.3%]	3

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

Binary size

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

Bootstrap: 657.866s -> 658.453s (0.09%)

oli-obk · 2023-10-25T13:49:26Z

closing in favor of #113169

Only call `mir_const_qualif` if absolutely necessary Pull the perf change out of rust-lang#113617 This should not have any impact on behaviour (if it does, we'll see an ICE)

…rochenkov Only call `mir_const_qualif` if absolutely necessary Pull the perf change out of rust-lang#113617 This should not have any impact on behaviour (if it does, we'll see an ICE)

Only call `mir_const_qualif` if absolutely necessary Pull the perf change out of rust-lang/rust#113617 This should not have any impact on behaviour (if it does, we'll see an ICE)

oli-obk added 3 commits July 12, 2023 10:08

Add regression test

2101271

Only call mir_const_qualif if absolutely necessary

55bfcb8

Pessimistically assume opaque types are !Freeze

b39f459

rustbot assigned joshtriplett Jul 12, 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 Jul 12, 2023

oli-obk added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 12, 2023

oli-obk commented Jul 12, 2023

View reviewed changes

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 Jul 12, 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 Jul 13, 2023

oli-obk closed this Oct 25, 2023

oli-obk mentioned this pull request Oct 25, 2023

Only call mir_const_qualif if absolutely necessary #117166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pessimistically assume opaque types are !Freeze #113617

Pessimistically assume opaque types are !Freeze #113617

oli-obk commented Jul 12, 2023

rustbot commented Jul 12, 2023

rustbot commented Jul 12, 2023

oli-obk Jul 12, 2023 •

edited

Loading

oli-obk commented Jul 12, 2023

This comment has been minimized.

bors commented Jul 12, 2023

bors commented Jul 12, 2023

This comment has been minimized.

lukas-code commented Jul 12, 2023

rust-timer commented Jul 13, 2023

oli-obk commented Oct 25, 2023

Pessimistically assume opaque types are !Freeze #113617

Pessimistically assume opaque types are !Freeze #113617

Conversation

oli-obk commented Jul 12, 2023

rustbot commented Jul 12, 2023

rustbot commented Jul 12, 2023

oli-obk Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Jul 12, 2023

This comment has been minimized.

bors commented Jul 12, 2023

bors commented Jul 12, 2023

This comment has been minimized.

lukas-code commented Jul 12, 2023

rust-timer commented Jul 13, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

oli-obk commented Oct 25, 2023

oli-obk Jul 12, 2023 •

edited

Loading