From 9d7295f0be2c90a98b6a7ce5f24ddef96dea10b7 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 15 Jun 2023 09:37:36 +1000 Subject: [PATCH 1/4] Move dead CGU marking code out of `partition`. The other major steps in `partition` have their own function, so it's nice for this one to be likewise. --- .../rustc_monomorphize/src/partitioning.rs | 55 ++++++++++--------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index ebcc3b0399973..2a314727744f8 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -197,34 +197,8 @@ where let instrument_dead_code = tcx.sess.instrument_coverage() && !tcx.sess.instrument_coverage_except_unused_functions(); - if instrument_dead_code { - assert!( - codegen_units.len() > 0, - "There must be at least one CGU that code coverage data can be generated in." - ); - - // Find the smallest CGU that has exported symbols and put the dead - // function stubs in that CGU. We look for exported symbols to increase - // the likelihood the linker won't throw away the dead functions. - // FIXME(#92165): In order to truly resolve this, we need to make sure - // the object file (CGU) containing the dead function stubs is included - // in the final binary. This will probably require forcing these - // function symbols to be included via `-u` or `/include` linker args. - let mut cgus: Vec<_> = codegen_units.iter_mut().collect(); - cgus.sort_by_key(|cgu| cgu.size_estimate()); - - let dead_code_cgu = - if let Some(cgu) = cgus.into_iter().rev().find(|cgu| { - cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External) - }) { - cgu - } else { - // If there are no CGUs that have externally linked items, - // then we just pick the first CGU as a fallback. - &mut codegen_units[0] - }; - dead_code_cgu.make_code_coverage_dead_code_cgu(); + mark_code_coverage_dead_code_cgu(&mut codegen_units); } // Ensure CGUs are sorted by name, so that we get deterministic results. @@ -545,6 +519,33 @@ fn internalize_symbols<'tcx>( } } +fn mark_code_coverage_dead_code_cgu<'tcx>(codegen_units: &mut [CodegenUnit<'tcx>]) { + assert!(!codegen_units.is_empty()); + + // Find the smallest CGU that has exported symbols and put the dead + // function stubs in that CGU. We look for exported symbols to increase + // the likelihood the linker won't throw away the dead functions. + // FIXME(#92165): In order to truly resolve this, we need to make sure + // the object file (CGU) containing the dead function stubs is included + // in the final binary. This will probably require forcing these + // function symbols to be included via `-u` or `/include` linker args. + let mut cgus: Vec<&mut CodegenUnit<'tcx>> = codegen_units.iter_mut().collect(); + cgus.sort_by_key(|cgu| cgu.size_estimate()); + + let dead_code_cgu = if let Some(cgu) = cgus + .into_iter() + .rev() + .find(|cgu| cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External)) + { + cgu + } else { + // If there are no CGUs that have externally linked items, + // then we just pick the first CGU as a fallback. + &mut codegen_units[0] + }; + dead_code_cgu.make_code_coverage_dead_code_cgu(); +} + fn characteristic_def_id_of_mono_item<'tcx>( tcx: TyCtxt<'tcx>, mono_item: MonoItem<'tcx>, From 57a7c8f577abd12ef4bb404448c42d794a76c690 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 15 Jun 2023 09:50:48 +1000 Subject: [PATCH 2/4] Fix bug in `mark_code_coverage_dead_code_cgus`. The comment says "Find the smallest CGU that has exported symbols and put the dead function stubs in that CGU". But the code sorts the CGUs by size (smallest first) and then searches them in reverse order, which means it will find the *largest* CGU that has exported symbols. The erroneous code was introduced in #92142. This commit changes it to use a simpler search, avoiding the sort, and fixes the bug in the process. --- .../rustc_monomorphize/src/partitioning.rs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 2a314727744f8..de57992beac33 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -529,20 +529,15 @@ fn mark_code_coverage_dead_code_cgu<'tcx>(codegen_units: &mut [CodegenUnit<'tcx> // the object file (CGU) containing the dead function stubs is included // in the final binary. This will probably require forcing these // function symbols to be included via `-u` or `/include` linker args. - let mut cgus: Vec<&mut CodegenUnit<'tcx>> = codegen_units.iter_mut().collect(); - cgus.sort_by_key(|cgu| cgu.size_estimate()); + let dead_code_cgu = codegen_units + .iter_mut() + .filter(|cgu| cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External)) + .min_by_key(|cgu| cgu.size_estimate()); + + // If there are no CGUs that have externally linked items, then we just + // pick the first CGU as a fallback. + let dead_code_cgu = if let Some(cgu) = dead_code_cgu { cgu } else { &mut codegen_units[0] }; - let dead_code_cgu = if let Some(cgu) = cgus - .into_iter() - .rev() - .find(|cgu| cgu.items().iter().any(|(_, (linkage, _))| *linkage == Linkage::External)) - { - cgu - } else { - // If there are no CGUs that have externally linked items, - // then we just pick the first CGU as a fallback. - &mut codegen_units[0] - }; dead_code_cgu.make_code_coverage_dead_code_cgu(); } From e414d25e94b8cfc29ee2ad44af10e2fd6644d36b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 15 Jun 2023 10:00:55 +1000 Subject: [PATCH 3/4] Make `partition` more consistent. Always put the `create_size_estimate` calls and `debug_dump` calls within a timed scopes. This makes the four main steps look more similar to each other. --- .../rustc_monomorphize/src/partitioning.rs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index de57992beac33..a74ba8e4a4be9 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -155,14 +155,16 @@ where // functions and statics defined in the local crate. let PlacedRootMonoItems { mut codegen_units, internalization_candidates, unique_inlined_stats } = { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_roots"); - place_root_mono_items(cx, mono_items) - }; + let mut placed = place_root_mono_items(cx, mono_items); - for cgu in &mut codegen_units { - cgu.create_size_estimate(tcx); - } + for cgu in &mut placed.codegen_units { + cgu.create_size_estimate(tcx); + } - debug_dump(tcx, "ROOTS", &codegen_units, unique_inlined_stats); + debug_dump(tcx, "ROOTS", &placed.codegen_units, placed.unique_inlined_stats); + + placed + }; // Merge until we have at most `max_cgu_count` codegen units. // `merge_codegen_units` is responsible for updating the CGU size @@ -179,22 +181,25 @@ where // local functions the definition of which is marked with `#[inline]`. { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_inline_items"); - place_inlined_mono_items(cx, &mut codegen_units) - }; + place_inlined_mono_items(cx, &mut codegen_units); - for cgu in &mut codegen_units { - cgu.create_size_estimate(tcx); - } + for cgu in &mut codegen_units { + cgu.create_size_estimate(tcx); + } - debug_dump(tcx, "INLINE", &codegen_units, unique_inlined_stats); + debug_dump(tcx, "INLINE", &codegen_units, unique_inlined_stats); + } // Next we try to make as many symbols "internal" as possible, so LLVM has // more freedom to optimize. if !tcx.sess.link_dead_code() { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_internalize_symbols"); internalize_symbols(cx, &mut codegen_units, internalization_candidates); + + debug_dump(tcx, "INTERNALIZE", &codegen_units, unique_inlined_stats); } + // Mark one CGU for dead code, if necessary. let instrument_dead_code = tcx.sess.instrument_coverage() && !tcx.sess.instrument_coverage_except_unused_functions(); if instrument_dead_code { @@ -204,8 +209,6 @@ where // Ensure CGUs are sorted by name, so that we get deterministic results. assert!(codegen_units.is_sorted_by(|a, b| Some(a.name().as_str().cmp(b.name().as_str())))); - debug_dump(tcx, "FINAL", &codegen_units, unique_inlined_stats); - codegen_units } From 2af5f2276d0cc2b34f2304415b1fbd6ccd42bc49 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 15 Jun 2023 18:58:23 +1000 Subject: [PATCH 4/4] Merge CGUs in a nicer way. --- compiler/rustc_monomorphize/src/partitioning.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index a74ba8e4a4be9..531644f0b8490 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -340,9 +340,7 @@ fn merge_codegen_units<'tcx>( // Move the mono-items from `smallest` to `second_smallest` second_smallest.modify_size_estimate(smallest.size_estimate()); - for (k, v) in smallest.items_mut().drain() { - second_smallest.items_mut().insert(k, v); - } + second_smallest.items_mut().extend(smallest.items_mut().drain()); // Record that `second_smallest` now contains all the stuff that was // in `smallest` before.