From 0a1cd5baa4d0ad4994f4038988a76770a6e2150c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 2 Jun 2023 11:07:38 +1000 Subject: [PATCH 1/7] Remove some unnecessary `&mut`s. --- compiler/rustc_monomorphize/src/partitioning.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 79fcd62bc6206..e99551410e022 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -136,7 +136,7 @@ struct PlacedRootMonoItems<'tcx> { // The output CGUs are sorted by name. fn partition<'tcx, I>( tcx: TyCtxt<'tcx>, - mono_items: &mut I, + mono_items: I, max_cgu_count: usize, usage_map: &UsageMap<'tcx>, ) -> Vec> @@ -239,7 +239,7 @@ where fn place_root_mono_items<'tcx, I>( cx: &PartitioningCx<'_, 'tcx>, - mono_items: &mut I, + mono_items: I, ) -> PlacedRootMonoItems<'tcx> where I: Iterator>, @@ -951,12 +951,8 @@ fn collect_and_partition_mono_items(tcx: TyCtxt<'_>, (): ()) -> (&DefIdSet, &[Co let (codegen_units, _) = tcx.sess.time("partition_and_assert_distinct_symbols", || { sync::join( || { - let mut codegen_units = partition( - tcx, - &mut items.iter().copied(), - tcx.sess.codegen_units(), - &usage_map, - ); + let mut codegen_units = + partition(tcx, items.iter().copied(), tcx.sess.codegen_units(), &usage_map); codegen_units[0].make_primary(); &*tcx.arena.alloc_from_iter(codegen_units) }, From 9fd6d979152db51271b12ad66ab7e8352e2bbd4f Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 2 Jun 2023 14:12:05 +1000 Subject: [PATCH 2/7] Improve sorting in `debug_dump`. Currently it sorts by symbol name, which is a mangled name like `_ZN1a4main17hb29587cdb6db5f42E`, which leads to non-obvious orderings. This commit changes it to use the existing `items_in_deterministic_order`, which iterates in source code order. --- compiler/rustc_monomorphize/src/partitioning.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index e99551410e022..2909042a931d1 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -864,15 +864,10 @@ fn debug_dump<'a, 'tcx: 'a>(tcx: TyCtxt<'tcx>, label: &str, cgus: &[CodegenUnit< cgu.size_estimate() ); - // The order of `cgu.items()` is non-deterministic; sort it by name - // to give deterministic output. - let mut items: Vec<_> = cgu.items().iter().collect(); - items.sort_by_key(|(item, _)| item.symbol_name(tcx).name); - for (item, linkage) in items { + for (item, linkage) in cgu.items_in_deterministic_order(tcx) { let symbol_name = item.symbol_name(tcx).name; let symbol_hash_start = symbol_name.rfind('h'); let symbol_hash = symbol_hash_start.map_or("", |i| &symbol_name[i..]); - let size = item.size_estimate(tcx); let _ = with_no_trimmed_paths!(writeln!( s, From 392045b7e70cc68290851451994d4ea135ec3f0a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Jun 2023 12:01:53 +1000 Subject: [PATCH 3/7] Make the two loops in `internalize_symbols` have the same form. Because the next commit will merge them. --- compiler/rustc_monomorphize/src/partitioning.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 2909042a931d1..fb8c21c59f22b 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -491,11 +491,14 @@ fn internalize_symbols<'tcx>( // can internalize all candidates, since there is nowhere else they // could be used from. for cgu in codegen_units { - for candidate in &internalization_candidates { - cgu.items_mut().insert(*candidate, (Linkage::Internal, Visibility::Default)); + for (item, linkage_and_visibility) in cgu.items_mut() { + if !internalization_candidates.contains(item) { + // This item is no candidate for internalizing, so skip it. + continue; + } + *linkage_and_visibility = (Linkage::Internal, Visibility::Default); } } - return; } From fe3b6465654d8d73f9ddd45faf02971614f5780b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Jun 2023 16:05:48 +1000 Subject: [PATCH 4/7] Merge the two loops in `internalize_symbols`. Because they have a lot of overlap. --- .../rustc_monomorphize/src/partitioning.rs | 49 +++++++------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index fb8c21c59f22b..c88df1b0b60f3 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -486,21 +486,7 @@ fn internalize_symbols<'tcx>( mono_item_placements: FxHashMap, MonoItemPlacement>, internalization_candidates: FxHashSet>, ) { - if codegen_units.len() == 1 { - // Fast path for when there is only one codegen unit. In this case we - // can internalize all candidates, since there is nowhere else they - // could be used from. - for cgu in codegen_units { - for (item, linkage_and_visibility) in cgu.items_mut() { - if !internalization_candidates.contains(item) { - // This item is no candidate for internalizing, so skip it. - continue; - } - *linkage_and_visibility = (Linkage::Internal, Visibility::Default); - } - } - return; - } + let single_codegen_unit = codegen_units.len() == 1; // For each internalization candidates in each codegen unit, check if it is // used from outside its defining codegen unit. @@ -512,21 +498,24 @@ fn internalize_symbols<'tcx>( // This item is no candidate for internalizing, so skip it. continue; } - debug_assert_eq!(mono_item_placements[item], home_cgu); - - if let Some(user_items) = cx.usage_map.get_user_items(*item) { - if user_items - .iter() - .filter_map(|user_item| { - // Some user mono items might not have been - // instantiated. We can safely ignore those. - mono_item_placements.get(user_item) - }) - .any(|placement| *placement != home_cgu) - { - // Found a user from another CGU, so skip to the next item - // without marking this one as internal. - continue; + + if !single_codegen_unit { + debug_assert_eq!(mono_item_placements[item], home_cgu); + + if let Some(user_items) = cx.usage_map.get_user_items(*item) { + if user_items + .iter() + .filter_map(|user_item| { + // Some user mono items might not have been + // instantiated. We can safely ignore those. + mono_item_placements.get(user_item) + }) + .any(|placement| *placement != home_cgu) + { + // Found a user from another CGU, so skip to the next item + // without marking this one as internal. + continue; + } } } From 8dbb3475b93a15cbd64e9b726a775ee2b31328a6 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 7 Jun 2023 09:24:37 +1000 Subject: [PATCH 5/7] Split loop in `place_inlined_mono_item`. This loop is doing two different things. For inlined items, it's adding them to the CGU. For all items, it's recording them in `mono_item_placements`. This commit splits it into two separate loops. This avoids putting root mono items into `reachable`, and removes the low-value check that `roots` doesn't contain inlined mono items. --- .../rustc_monomorphize/src/partitioning.rs | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index c88df1b0b60f3..737c0b035e7ab 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -416,37 +416,35 @@ enum MonoItemPlacement { fn place_inlined_mono_items<'tcx>( cx: &PartitioningCx<'_, 'tcx>, codegen_units: &mut [CodegenUnit<'tcx>], - roots: FxHashSet>, + _roots: FxHashSet>, ) -> FxHashMap, MonoItemPlacement> { - let mut mono_item_placements = FxHashMap::default(); - - let single_codegen_unit = codegen_units.len() == 1; - for cgu in codegen_units.iter_mut() { - // Collect all items that need to be available in this codegen unit. - let mut reachable = FxHashSet::default(); + // Collect all inlined items that need to be available in this codegen unit. + let mut reachable_inlined_items = FxHashSet::default(); for root in cgu.items().keys() { - // Insert the root item itself, plus all inlined items that are - // reachable from it without going via another root item. - reachable.insert(*root); - get_reachable_inlined_items(cx.tcx, *root, cx.usage_map, &mut reachable); + // Get all inlined items that are reachable from it without going + // via another root item. + get_reachable_inlined_items(cx.tcx, *root, cx.usage_map, &mut reachable_inlined_items); } // Add all monomorphizations that are not already there. - for mono_item in reachable { - if !cgu.items().contains_key(&mono_item) { - if roots.contains(&mono_item) { - bug!("GloballyShared mono-item inlined into other CGU: {:?}", mono_item); - } + for inlined_item in reachable_inlined_items { + assert!(!cgu.items().contains_key(&inlined_item)); - // This is a CGU-private copy. - cgu.items_mut().insert(mono_item, (Linkage::Internal, Visibility::Default)); - } + // This is a CGU-private copy. + cgu.items_mut().insert(inlined_item, (Linkage::Internal, Visibility::Default)); + } + } + + let mut mono_item_placements = FxHashMap::default(); + let single_codegen_unit = codegen_units.len() == 1; + for cgu in codegen_units.iter_mut() { + for item in cgu.items().keys() { if !single_codegen_unit { // If there is more than one codegen unit, we need to keep track // in which codegen units each monomorphization is placed. - match mono_item_placements.entry(mono_item) { + match mono_item_placements.entry(*item) { Entry::Occupied(e) => { let placement = e.into_mut(); debug_assert!(match *placement { From 1defd3076473699332e1b4c424d35f0dfb28c63b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 7 Jun 2023 09:24:51 +1000 Subject: [PATCH 6/7] Remove `PlacedRootMonoItems::roots`. It's no longer used. --- compiler/rustc_monomorphize/src/partitioning.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 737c0b035e7ab..e3a226fd427fa 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -129,7 +129,6 @@ struct PlacedRootMonoItems<'tcx> { /// The codegen units, sorted by name to make things deterministic. codegen_units: Vec>, - roots: FxHashSet>, internalization_candidates: FxHashSet>, } @@ -150,7 +149,7 @@ where // In the first step, we place all regular monomorphizations into their // respective 'home' codegen unit. Regular monomorphizations are all // functions and statics defined in the local crate. - let PlacedRootMonoItems { mut codegen_units, roots, internalization_candidates } = { + let PlacedRootMonoItems { mut codegen_units, internalization_candidates } = { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_roots"); place_root_mono_items(cx, mono_items) }; @@ -176,7 +175,7 @@ where // local functions the definition of which is marked with `#[inline]`. let mono_item_placements = { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_inline_items"); - place_inlined_mono_items(cx, &mut codegen_units, roots) + place_inlined_mono_items(cx, &mut codegen_units) }; for cgu in &mut codegen_units { @@ -244,7 +243,6 @@ fn place_root_mono_items<'tcx, I>( where I: Iterator>, { - let mut roots = FxHashSet::default(); let mut codegen_units = FxHashMap::default(); let is_incremental_build = cx.tcx.sess.opts.incremental.is_some(); let mut internalization_candidates = FxHashSet::default(); @@ -295,7 +293,6 @@ where } codegen_unit.items_mut().insert(mono_item, (linkage, visibility)); - roots.insert(mono_item); } // Always ensure we have at least one CGU; otherwise, if we have a @@ -308,7 +305,7 @@ where let mut codegen_units: Vec<_> = codegen_units.into_values().collect(); codegen_units.sort_by(|a, b| a.name().as_str().cmp(b.name().as_str())); - PlacedRootMonoItems { codegen_units, roots, internalization_candidates } + PlacedRootMonoItems { codegen_units, internalization_candidates } } // This function requires the CGUs to be sorted by name on input, and ensures @@ -416,7 +413,6 @@ enum MonoItemPlacement { fn place_inlined_mono_items<'tcx>( cx: &PartitioningCx<'_, 'tcx>, codegen_units: &mut [CodegenUnit<'tcx>], - _roots: FxHashSet>, ) -> FxHashMap, MonoItemPlacement> { for cgu in codegen_units.iter_mut() { // Collect all inlined items that need to be available in this codegen unit. From 853345635be1c51f180174d0ed66bb85aacc2570 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 7 Jun 2023 10:59:00 +1000 Subject: [PATCH 7/7] Move `mono_item_placement` construction. It's currently created in `place_inlined_mono_items` and then used in `internalize_symbols`. This commit moves the creation to `internalize_symbols`. --- .../rustc_monomorphize/src/partitioning.rs | 80 ++++++++----------- 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index e3a226fd427fa..1d9c8ded349c0 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -173,7 +173,7 @@ where // monomorphizations have to go into each codegen unit. These additional // monomorphizations can be drop-glue, functions from external crates, and // local functions the definition of which is marked with `#[inline]`. - let mono_item_placements = { + { let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_inline_items"); place_inlined_mono_items(cx, &mut codegen_units) }; @@ -188,12 +188,7 @@ where // 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, - mono_item_placements, - internalization_candidates, - ); + internalize_symbols(cx, &mut codegen_units, internalization_candidates); } let instrument_dead_code = @@ -401,19 +396,10 @@ fn merge_codegen_units<'tcx>( codegen_units.sort_by(|a, b| a.name().as_str().cmp(b.name().as_str())); } -/// For symbol internalization, we need to know whether a symbol/mono-item is -/// used from outside the codegen unit it is defined in. This type is used -/// to keep track of that. -#[derive(Clone, PartialEq, Eq, Debug)] -enum MonoItemPlacement { - SingleCgu { cgu_name: Symbol }, - MultipleCgus, -} - fn place_inlined_mono_items<'tcx>( cx: &PartitioningCx<'_, 'tcx>, codegen_units: &mut [CodegenUnit<'tcx>], -) -> FxHashMap, MonoItemPlacement> { +) { for cgu in codegen_units.iter_mut() { // Collect all inlined items that need to be available in this codegen unit. let mut reachable_inlined_items = FxHashSet::default(); @@ -432,33 +418,6 @@ fn place_inlined_mono_items<'tcx>( } } - let mut mono_item_placements = FxHashMap::default(); - let single_codegen_unit = codegen_units.len() == 1; - - for cgu in codegen_units.iter_mut() { - for item in cgu.items().keys() { - if !single_codegen_unit { - // If there is more than one codegen unit, we need to keep track - // in which codegen units each monomorphization is placed. - match mono_item_placements.entry(*item) { - Entry::Occupied(e) => { - let placement = e.into_mut(); - debug_assert!(match *placement { - MonoItemPlacement::SingleCgu { cgu_name } => cgu_name != cgu.name(), - MonoItemPlacement::MultipleCgus => true, - }); - *placement = MonoItemPlacement::MultipleCgus; - } - Entry::Vacant(e) => { - e.insert(MonoItemPlacement::SingleCgu { cgu_name: cgu.name() }); - } - } - } - } - } - - return mono_item_placements; - fn get_reachable_inlined_items<'tcx>( tcx: TyCtxt<'tcx>, item: MonoItem<'tcx>, @@ -477,11 +436,42 @@ fn place_inlined_mono_items<'tcx>( fn internalize_symbols<'tcx>( cx: &PartitioningCx<'_, 'tcx>, codegen_units: &mut [CodegenUnit<'tcx>], - mono_item_placements: FxHashMap, MonoItemPlacement>, internalization_candidates: FxHashSet>, ) { + /// For symbol internalization, we need to know whether a symbol/mono-item + /// is used from outside the codegen unit it is defined in. This type is + /// used to keep track of that. + #[derive(Clone, PartialEq, Eq, Debug)] + enum MonoItemPlacement { + SingleCgu { cgu_name: Symbol }, + MultipleCgus, + } + + let mut mono_item_placements = FxHashMap::default(); let single_codegen_unit = codegen_units.len() == 1; + if !single_codegen_unit { + for cgu in codegen_units.iter_mut() { + for item in cgu.items().keys() { + // If there is more than one codegen unit, we need to keep track + // in which codegen units each monomorphization is placed. + match mono_item_placements.entry(*item) { + Entry::Occupied(e) => { + let placement = e.into_mut(); + debug_assert!(match *placement { + MonoItemPlacement::SingleCgu { cgu_name } => cgu_name != cgu.name(), + MonoItemPlacement::MultipleCgus => true, + }); + *placement = MonoItemPlacement::MultipleCgus; + } + Entry::Vacant(e) => { + e.insert(MonoItemPlacement::SingleCgu { cgu_name: cgu.name() }); + } + } + } + } + } + // For each internalization candidates in each codegen unit, check if it is // used from outside its defining codegen unit. for cgu in codegen_units {