Report monomorphization time errors in dead code, too #112879

oli-obk · 2023-06-21T07:40:26Z

Before this PR

struct Foo<T>(T);

impl<T> Foo<T> {
    const BAR: () = if std::mem::size_of::<T>() == 0 {
        panic!()
        //~^ ERROR evaluation of `Foo::<()>::BAR` failed
    };
}

#[inline(never)]
fn bop<T>() {
    Foo::<T>::BAR;
}

fn fop<T>() {
    if false {
        bop::<T>();
    }
}

fn main() {
    fop::<()>();
}

would only error in debug mode, but pass in release mode. This behaviour is a blocker for the stabilization of inline_const.

TODO:

test performance impact
self-review PR

Fixes #107503

rustbot · 2023-06-21T07:40:30Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

oli-obk · 2023-06-21T07:40:37Z

@bors try @rust-timer queue

bors · 2023-06-21T07:40:46Z

⌛ Trying commit b0f2ef5c94d4e15efafbda590402e83dd9f10dd3 with merge ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1...

rustbot · 2023-06-21T08:03:10Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

oli-obk · 2023-06-21T09:21:39Z

@rust-timer build ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1

rust-timer · 2023-06-21T11:28:22Z

Finished benchmarking commit (ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	7.3%	[0.4%, 39.8%]	224
Regressions ❌ (secondary)	10.1%	[0.3%, 75.9%]	175
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.3%]	4
All ❌✅ (primary)	7.3%	[0.4%, 39.8%]	224

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)	7.6%	[0.8%, 28.8%]	187
Regressions ❌ (secondary)	6.5%	[1.8%, 21.9%]	152
Improvements ✅ (primary)	-3.3%	[-4.3%, -2.3%]	2
Improvements ✅ (secondary)	-1.9%	[-2.2%, -1.5%]	3
All ❌✅ (primary)	7.5%	[-4.3%, 28.8%]	189

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)	11.3%	[1.1%, 43.2%]	144
Regressions ❌ (secondary)	14.5%	[0.8%, 83.2%]	107
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	11.3%	[1.1%, 43.2%]	144

Binary size

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)	15.0%	[0.7%, 35.0%]	140
Regressions ❌ (secondary)	20.5%	[0.1%, 95.4%]	89
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	15.0%	[0.7%, 35.0%]	140

Bootstrap: 655.662s -> 705.23s (7.56%)

oli-obk · 2023-06-21T14:23:10Z

@bors try @rust-timer queue

bors · 2023-06-21T14:23:21Z

⌛ Trying commit 95f65cdc06bfa1cda8d9696e8692c8bde74c9041 with merge 8ccd86a2870ef85c4183d273a94f5b546a599126...

tmiasko · 2023-06-21T15:44:00Z

compiler/rustc_middle/src/mir/mod.rs

+    /// The `bool` states whether this is a direct call or not.
+    Fn(DefId, SubstsRef<'tcx>, bool),
+    Static(DefId),
+    GlobalAsm(LocalDefId),


GlobalAsm is unused.

traviscross · 2023-10-24T20:01:54Z

@rustbot labels -I-lang-nominated

I'm going to unnominate this, as there's nothing more for T-lang to discuss until this is investigated. For anyone who picks this up, please renominate this after posting the investigation (or fix).

Try caching the collect_and_partition_mono_items query r? `@ghost` an experiment to debug some performance issues encountered in rust-lang#112879

the8472 · 2024-03-07T22:57:09Z

Based on conversation in rust-lang/rfcs#3582 I have modified the PR example by moving the dead branch into const eval instead of a regular function.

struct Foo<T, const N: usize>(T);

impl<T, const N: usize> Foo<T, N> {
    const BAR: () = if N == 0 {
        panic!()
    };
}

struct Invoke<T, const N: usize>(T);

impl<T, const N: usize> Invoke<T, N> {
    const FUN: fn() = if N != 0 {
        || Foo::<T, N>::BAR
    } else {
        || {}
    };
}

fn main() {
    Invoke::<(), 0>::FUN();
}

This passes on stable with and without optimizations. Would this PR cause it to fail?

Draft: monomorphize things from dead code, too This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the perf regression comes from. So here I want to take baby steps to see the impact of each step. r? `@ghost`

RalfJung · 2024-03-10T11:49:36Z

I started experimenting a bit in #122258 to see which step costs how much perf.

But I think I also better understand now how const-eval errors behave during monomorphization. There are actually two stages of monomorphization: collect all the things we want to codegen, and then codegen them all. These seem to happen sequentially, and if there is an error during collection we never go to codegen. Const-eval errors are triggered during collection (the collector triggers the const-eval queries).

Our goal (to unblock const {}) is just to detect const-eval errors in dead code, not errors that happen later during codegen. I think this PR will make dead functions fully monomorphized, i.e., they are actually passed to codegen -- when all that we really want is to walk them and trigger evaluation of all consts that we can find (recursively). Perf will probably look a lot better if we can avoid sending these functions to codegen. The monomorphization collection just has a single walking mode though, "walk everything, evaluate consts and add functions to the codegen list".

I think we either want to change monomorphization to walk all required_items without sending them to codegen (only send them to codegen if we also find the same item in the MIR), or we want a separate traversal that does the recursive search for all consts. That separate traversal would be very similar to the monomorphization traversal though so this doesn't sound great, we'd probably duplicate a lot of effort and code. Making monomorphization keep track of whether we are currently visiting "for const eval only" or "for codegen" could also become a bit messy but still seems more promising IMO.

I haven't looked into monomorphization before so take this with a bunch of salt.

RalfJung · 2024-03-10T11:56:19Z

@the8472 We'd have to decide whether dead code in const is monomorphized. Currently it is not, and I think this PR doesn't change that. This is not the result of optimizations; the collector simply does not walk the MIR of a const and const-eval only monomorphizes functions that actually get called. We could make const-eval iterate over required_items and recursively search them for consts to evaluate, then your example would fail to build. I think whether or not we want that is largely orthogonal to this PR, and it shouldn't be on the critical path for const {} as it is already not optimization-dependent. So please open a new issue. :)

tl;dr

Would this PR cause it to fail?

No, but there's also AFAIK no guarantee that this won't fail in the future.

Draft: monomorphize things from dead code, too This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. So here I want to take baby steps to see the impact of each step. r? `@ghost`

Draft: recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang#122258 I learned some things, which informed the approach this PR is taking. r? `@ghost`

recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` Fixes rust-lang#107503

RalfJung · 2024-03-17T13:39:07Z

I think this is superseded by #122568. I managed to get the perf overhead down to 3% for the worst case, and it almost exclusively affects incr builds (which, in the benchmark suite, are "incr with no or almost no change").

recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` Fixes rust-lang#107503

recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. //! //! The collector and "mentioned items" gathering (which lives in `rustc_mir_transform::mentioned_items`) //! need to stay in sync in the following sense: //! //! - For every item that the collector gather that could eventually lead to build failure (most //! likely due to containing a constant that fails to evaluate), a corresponding mentioned item //! must be added. This should use the exact same strategy as the ecollector to make sure they are //! in sync. However, while the collector works on monomorphized types, mentioned items are //! collected on generic MIR -- so any time the collector checks for a particular type (such as //! `ty::FnDef`), we have to just onconditionally add this as a mentioned item. //! - In `visit_mentioned_item`, we then do with that mentioned item exactly what the collector //! would have done during regular MIR visiting. Basically you can think of the collector having //! two stages, a pre-monomorphization stage and a post-monomorphization stage (usually quite //! literally separated by a call to `self.monomorphize`); the pre-monomorphizationn stage is //! duplicated in mentioned items gathering and the post-monomorphization stage is duplicated in //! `visit_mentioned_item`. //! - Finally, as a performance optimization, the collector should fill `used_mentioned_item` during //! its MIR traversal with exactly what mentioned item gathering would have added in the same //! situation. This detects mentioned items that have *not* been optimized away and hence don't //! need a dedicated traversal. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` And the `mentioned_items` MIR body field docs: ```rust /// Further items that were mentioned in this function and hence *may* become monomorphized, /// depending on optimizations. We use this to avoid optimization-dependent compile errors: the /// collector recursively traverses all "mentioned" items and evaluates all their /// `required_consts`. /// /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. /// All that's relevant is that this set is optimization-level-independent, and that it includes /// everything that the collector would consider "used". (For example, we currently compute this /// set after drop elaboration, so some drop calls that can never be reached are not considered /// "mentioned".) See the documentation of `CollectionMode` in /// `compiler/rustc_monomorphize/src/collector.rs` for more context. pub mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>, ``` Fixes rust-lang#107503

recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang/rust#107503. The previous attempt at rust-lang/rust#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang/rust#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. //! //! The collector and "mentioned items" gathering (which lives in `rustc_mir_transform::mentioned_items`) //! need to stay in sync in the following sense: //! //! - For every item that the collector gather that could eventually lead to build failure (most //! likely due to containing a constant that fails to evaluate), a corresponding mentioned item //! must be added. This should use the exact same strategy as the ecollector to make sure they are //! in sync. However, while the collector works on monomorphized types, mentioned items are //! collected on generic MIR -- so any time the collector checks for a particular type (such as //! `ty::FnDef`), we have to just onconditionally add this as a mentioned item. //! - In `visit_mentioned_item`, we then do with that mentioned item exactly what the collector //! would have done during regular MIR visiting. Basically you can think of the collector having //! two stages, a pre-monomorphization stage and a post-monomorphization stage (usually quite //! literally separated by a call to `self.monomorphize`); the pre-monomorphizationn stage is //! duplicated in mentioned items gathering and the post-monomorphization stage is duplicated in //! `visit_mentioned_item`. //! - Finally, as a performance optimization, the collector should fill `used_mentioned_item` during //! its MIR traversal with exactly what mentioned item gathering would have added in the same //! situation. This detects mentioned items that have *not* been optimized away and hence don't //! need a dedicated traversal. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` And the `mentioned_items` MIR body field docs: ```rust /// Further items that were mentioned in this function and hence *may* become monomorphized, /// depending on optimizations. We use this to avoid optimization-dependent compile errors: the /// collector recursively traverses all "mentioned" items and evaluates all their /// `required_consts`. /// /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. /// All that's relevant is that this set is optimization-level-independent, and that it includes /// everything that the collector would consider "used". (For example, we currently compute this /// set after drop elaboration, so some drop calls that can never be reached are not considered /// "mentioned".) See the documentation of `CollectionMode` in /// `compiler/rustc_monomorphize/src/collector.rs` for more context. pub mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>, ``` Fixes #107503

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 21, 2023