Substitute types before checking inlining compatibility. #113802

cjgillot · 2023-07-17T18:56:59Z

I don't have a minimal test, but I this seems to remove the ICE locally.

This whole pre-inlining validation mirrors the "real" MIR validation pass to verify that inlined MIR will still pass validation.
The debuginfo loop is added because MIR validation check projections in debuginfo.
Likewise, MIR validation only checks is_subtype, so there is no reason for a stronger check.

The types were not being substituted in check_equal, so we were not bailing out of inlining if the substituted MIR callee body would not pass validation.

rustbot · 2023-07-17T18:57:05Z

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-07-17T18:57:08Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2023-07-17T21:08:09Z

compiler/rustc_mir_transform/src/inline.rs

@@ -440,6 +440,10 @@ impl<'tcx> Inliner<'tcx> {
            validation: Ok(()),
        };

+        for var_debug_info in callee_body.var_debug_info.iter() {


What's this for?

(answered in: #113802 (comment) )

compiler-errors · 2023-07-17T21:09:04Z

compiler/rustc_mir_transform/src/inline.rs

+                if ty == f_ty {
+                    return;
+                }
+                if !util::is_subtype(this.tcx, this.param_env, ty, f_ty) {


Makes sense, but why are we changing this from is_equal_up_to_subtyping? I guess do we only want to check that ty <: f_ty?

(answered in: #113802 (comment) )

cjgillot · 2023-07-18T18:07:44Z

I should have put more context in the PR comment.
This whole pre-inlining validation mirrors the "real" MIR validation pass to verify that inlined MIR will still pass validation.
The debuginfo loop is added because MIR validation check projections in debuginfo.
Likewise, MIR validation only checks is_subtype, so there is no reason for a stronger check.

This ad-hoc validation should reuse the existing MIR validation code, and not brew its own, but that's for another PR.

compiler-errors · 2023-07-20T15:26:52Z

@bors r+

bors · 2023-07-20T15:26:54Z

📌 Commit 45ffe41 has been approved by compiler-errors

It is now in the queue for this repository.

cjgillot · 2023-07-20T16:35:04Z

I'm nominating for backport, as both ICEs affect stable 1.71.0.

bors · 2023-07-21T09:14:58Z

⌛ Testing commit 45ffe41 with merge 6b29036...

bors · 2023-07-21T11:02:41Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 6b29036 to master...

rust-timer · 2023-07-21T13:26:43Z

Finished benchmarking commit (6b29036): 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)	-	-	0
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-2.9%	[-3.0%, -2.7%]	2
All ❌✅ (primary)	-3.6%	[-3.6%, -3.6%]	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)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 651.291s -> 649.721s (-0.24%)

apiraino · 2023-07-28T12:43:19Z

Beta + stable backports approved as per compiler team on Zulip

@rustbot label +beta-accepted +stable-accepted

Prepare Rust 1.71.1 This PR prepares the Rust 1.71.1 release, which contains: * rust-lang#113802 * rust-lang#113579 * rust-lang#111516 * rust-lang#112517 * rust-lang@67b5990 (from rust-lang#113678) r? `@ghost` cc `@rust-lang/release`

[beta] backport * Restrict linker version script of proc-macro crates to just its two symbols rust-lang#114470 * bootstrap: config: fix version comparison bug rust-lang#114440 * lint/ctypes: only try normalize rust-lang#113921 * Avoid tls access while iterating through mpsc thread entries rust-lang#113861 * Substitute types before checking inlining compatibility. rust-lang#113802 * Revert "fix: bug etc/bash_complettion -> src/etc/... to avoid copy error" rust-lang#113579 * lint/ctypes: fix () return type checks rust-lang#113457 * Rename and allow cast_ref_to_mut lint rust-lang#113422 * Ignore flaky clippy tests. rust-lang#113621 r? cuviper

RalfJung · 2023-08-27T11:41:36Z

compiler/rustc_mir_transform/src/inline.rs

+                if ty == f_ty {
+                    return;
+                }
+                if !util::is_subtype(this.tcx, this.param_env, ty, f_ty) {


Would be good to add a comment pointing to the issue(s), to give some context to this check.

rustbot assigned compiler-errors Jul 17, 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 17, 2023

cjgillot changed the title ~~Substitute types before checking compatibility.~~ Substitute types before checking inlining compatibility. Jul 17, 2023

cjgillot added A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining labels Jul 17, 2023

compiler-errors reviewed Jul 17, 2023

View reviewed changes

Substitute types before checking compatibility.

45ffe41

cjgillot force-pushed the check-debuginfo branch from 6916910 to 45ffe41 Compare July 19, 2023 12:40

cjgillot mentioned this pull request Jul 19, 2023

Reuse the MIR validator for MIR inlining #113853

Merged

compiler-errors approved these changes Jul 20, 2023

View reviewed changes

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 Jul 20, 2023

cjgillot added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jul 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2023

bors merged commit 6b29036 into rust-lang:master Jul 21, 2023
12 checks passed

rustbot added this to the 1.73.0 milestone Jul 21, 2023

This was referenced Jul 21, 2023

Implement jump threading MIR opt #107009

Merged

[experiment] Use new solver in MIR validator subtyping checks #112365

Draft

cjgillot deleted the check-debuginfo branch July 21, 2023 13:54

rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jul 28, 2023

pietroalbini mentioned this pull request Jul 31, 2023

Prepare Rust 1.71.1 #114284

Merged

cuviper mentioned this pull request Aug 12, 2023

[beta] backport #114748

Merged

cuviper modified the milestones: 1.73.0, 1.72.0 Aug 12, 2023

cuviper removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Aug 12, 2023

cuviper modified the milestones: 1.72.0, 1.71.1 Aug 12, 2023

RalfJung mentioned this pull request Aug 27, 2023

inlining produces broken MIR by changing whether types can be normalized #112332

Open

RalfJung reviewed Aug 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Substitute types before checking inlining compatibility. #113802

Substitute types before checking inlining compatibility. #113802

cjgillot commented Jul 17, 2023 •

edited

rustbot commented Jul 17, 2023

rustbot commented Jul 17, 2023

compiler-errors Jul 17, 2023

pnkfelix Jul 27, 2023

compiler-errors Jul 17, 2023

pnkfelix Jul 27, 2023

cjgillot commented Jul 18, 2023

compiler-errors commented Jul 20, 2023

bors commented Jul 20, 2023

cjgillot commented Jul 20, 2023

bors commented Jul 21, 2023

bors commented Jul 21, 2023

rust-timer commented Jul 21, 2023

apiraino commented Jul 28, 2023

RalfJung Aug 27, 2023

Substitute types before checking inlining compatibility. #113802

Substitute types before checking inlining compatibility. #113802

Conversation

cjgillot commented Jul 17, 2023 • edited

rustbot commented Jul 17, 2023

rustbot commented Jul 17, 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

cjgillot commented Jul 18, 2023

compiler-errors commented Jul 20, 2023

bors commented Jul 20, 2023

cjgillot commented Jul 20, 2023

bors commented Jul 21, 2023

bors commented Jul 21, 2023

rust-timer commented Jul 21, 2023

Overall result: no relevant changes - no action needed

apiraino commented Jul 28, 2023

Choose a reason for hiding this comment

cjgillot commented Jul 17, 2023 •

edited