Stall computing instance for drop shim until it has no unsubstituted const params #127068

compiler-errors · 2024-06-28T02:15:12Z

Do not inline the drop shim for types that still have unsubstituted const params.

Why?

#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any Drop behavior):

rust/compiler/rustc_mir_dataflow/src/elaborate_drops.rs

Line 378 in 9c3bc80

self.place_ty(place).needs_drop(self.tcx(), self.elaborator.param_env())

However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env:

rust/compiler/rustc_mir_transform/src/shim.rs

Line 278 in 9c3bc80

let param_env = tcx.param_env_reveal_all_normalized(def_id);

(which is the param-env of std::ptr::drop_in_place)

This param-env is notably missing ConstParamHasTy predicates, and since we removed the type from consts in #125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators.

What?

We force the inliner to not consider calls for drop_in_place for types that have unsubstituted const params.

So what?

This may negatively affect MIR inlining, but I doubt this matters in practice, and fixes a beta regression, so let's fix it. I will look into approaches for fixing this in a more maintainable way, perhaps delaying the creation of drop shim bodies until codegen (like how intrinsics work).

rustbot · 2024-06-28T02:15:21Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

compiler-errors · 2024-06-28T02:15:26Z

r? @BoxyUwU

@bors try @rust-timer queue

Stall dropaStall computing instance for drop shim until it has no unsubstituted const params Stall resolving the drop shim instance for types that still have unsubstituted const params. ## Why? rust-lang#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any `Drop` behavior): https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_dataflow/src/elaborate_drops.rs#L378 However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env: https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_transform/src/shim.rs#L278 (which is the param-env of `std::ptr::drop_in_place`) This param-env is notably missing `ConstParamHasTy` predicates, and since we removed the type from consts in rust-lang#125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators. ## What? We delay the resolution (`Instance::resolve`) of calls for `drop_in_place` for types that have unsubstituted const params. This should be OK, since all cases that deal with polymorphic code should handle `Instance::resolve` returning `None` gracefully.

bors · 2024-06-28T02:16:37Z

⌛ Trying commit d62c70c with merge e495e73...

compiler-errors · 2024-06-28T02:37:55Z

compiler/rustc_ty_utils/src/instance.rs

@@ -53,6 +53,15 @@ fn resolve_instance<'tcx>(
                    _ => return Ok(None),
                }

+                // FIXME(#127030): `ConstParamHasTy` has bad interactions with


We could actually move this change to:

rust/compiler/rustc_mir_transform/src/inline.rs

Line 285 in 9c3bc80

fn check_mir_is_available(

But I am kinda worried that there are other calls to Instance::resolve that may trigger this problem. Not exactly sure why -- I could audit all of the Instance::resolve calls in the codebase, I guess.

Actually, I did check all the calls. The only one of note is actually in the inliner. I'm indifferent about where to put this hack, but I guess putting this hack in the inliner is likely to make less of a perf impact and easier to reason about for backports.

Let me move it after the perf run is done. Then I'll edit the description.

bors · 2024-06-28T03:57:44Z

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

rust-timer · 2024-06-28T05:13:40Z

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

Max RSS (memory usage)

Results (secondary 5.8%)

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

Cycles

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

Binary size

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

Bootstrap: 697.882s -> 696.928s (-0.14%)
Artifact size: 326.66 MiB -> 326.72 MiB (0.02%)

rustbot · 2024-06-28T14:18:35Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

BoxyUwU · 2024-06-28T19:17:48Z

@bors r+ rollup

thank you errs

bors · 2024-06-28T19:17:51Z

📌 Commit f17b27b has been approved by BoxyUwU

It is now in the queue for this repository.

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127016 (docs: check if the disambiguator matches its suffix) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) - rust-lang#127055 (Mark Hasher::finish as #[must_use]) - rust-lang#127068 (Stall computing instance for drop shim until it has no unsubstituted const params) - rust-lang#127070 (add () to the marker_impls macro for ConstParamTy) - rust-lang#127071 (Remove (deprecated & unstable) {to,from}_bits pointer methods) - rust-lang#127078 (Enable full tools and profiler for LoongArch Linux targets) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127068 - compiler-errors:stall-drop, r=BoxyUwU Stall computing instance for drop shim until it has no unsubstituted const params Do not inline the drop shim for types that still have unsubstituted const params. ## Why? rust-lang#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any `Drop` behavior): https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_dataflow/src/elaborate_drops.rs#L378 However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env: https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_transform/src/shim.rs#L278 (which is the param-env of `std::ptr::drop_in_place`) This param-env is notably missing `ConstParamHasTy` predicates, and since we removed the type from consts in rust-lang#125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators. ## What? We force the inliner to not consider calls for `drop_in_place` for types that have unsubstituted const params. ## So what? This may negatively affect MIR inlining, but I doubt this matters in practice, and fixes a beta regression, so let's fix it. I will look into approaches for fixing this in a more maintainable way, perhaps delaying the creation of drop shim bodies until codegen (like how intrinsics work).

apiraino · 2024-07-04T14:36:04Z

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

[beta] backports - Properly gate `safe` keyword in pre-expansion rust-lang#126757 - Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015 - Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068 - Update LLVM submodule rust-lang#127190 r? cuviper

[beta] backports - Properly gate `safe` keyword in pre-expansion rust-lang#126757 - Switch back `non_local_definitions` lint to allow-by-default rust-lang#127015 - Stall computing instance for drop shim until it has no unsubstituted const params rust-lang#127068 - Update LLVM submodule rust-lang#127190 - Change to the NetBSD archive URL rather than the CDN rust-lang#127232 r? cuviper

rustbot assigned TaKO8Ki Jun 28, 2024

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 Jun 28, 2024

rustbot assigned BoxyUwU Jun 28, 2024

This comment has been minimized.

Sign in to view

rustbot unassigned TaKO8Ki Jun 28, 2024

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 28, 2024

compiler-errors changed the title ~~Stall dropaStall computing instance for drop shim until it has no unsubstituted const params~~ Stall computing instance for drop shim until it has no unsubstituted const params Jun 28, 2024

compiler-errors commented Jun 28, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors added 2 commits June 28, 2024 10:18

Failing test for computing drop shim that has const param

90c2b23

Don't inline drop shims with unsubstituted generic consts in MIR inliner

f17b27b

compiler-errors force-pushed the stall-drop branch from d62c70c to f17b27b Compare June 28, 2024 14:18

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 Jun 28, 2024

matthiaskrgr mentioned this pull request Jun 28, 2024

Rollup of 11 pull requests #127096

Merged

bors merged commit 5afb4c2 into rust-lang:master Jun 29, 2024
6 checks passed

rustbot added this to the 1.81.0 milestone Jun 29, 2024

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 4, 2024

cuviper mentioned this pull request Jul 5, 2024

[beta] backports #127377

Merged

cuviper modified the milestones: 1.81.0, 1.80.0 Jul 5, 2024

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stall computing instance for drop shim until it has no unsubstituted const params #127068

Stall computing instance for drop shim until it has no unsubstituted const params #127068

compiler-errors commented Jun 28, 2024 •

edited

Loading

rustbot commented Jun 28, 2024

compiler-errors commented Jun 28, 2024

This comment has been minimized.

bors commented Jun 28, 2024

compiler-errors Jun 28, 2024

compiler-errors Jun 28, 2024

compiler-errors Jun 28, 2024 •

edited

Loading

bors commented Jun 28, 2024

This comment has been minimized.

rust-timer commented Jun 28, 2024

rustbot commented Jun 28, 2024

BoxyUwU commented Jun 28, 2024

bors commented Jun 28, 2024

apiraino commented Jul 4, 2024

Stall computing instance for drop shim until it has no unsubstituted const params #127068

Stall computing instance for drop shim until it has no unsubstituted const params #127068

Conversation

compiler-errors commented Jun 28, 2024 • edited Loading

Why?

What?

So what?

rustbot commented Jun 28, 2024

compiler-errors commented Jun 28, 2024

This comment has been minimized.

bors commented Jun 28, 2024

compiler-errors Jun 28, 2024

Choose a reason for hiding this comment

compiler-errors Jun 28, 2024

Choose a reason for hiding this comment

compiler-errors Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

bors commented Jun 28, 2024

This comment has been minimized.

rust-timer commented Jun 28, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

rustbot commented Jun 28, 2024

BoxyUwU commented Jun 28, 2024

bors commented Jun 28, 2024

apiraino commented Jul 4, 2024

compiler-errors commented Jun 28, 2024 •

edited

Loading

compiler-errors Jun 28, 2024 •

edited

Loading