From a13be655a511c215cc4d1d120d981bfe80b04eec Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 08:14:29 +1000 Subject: [PATCH 1/9] Avoid unnecessary line lookup. `lookup_debug_loc` calls `SourceMap::lookup_line`, which does a binary search over the files, and then a binary search over the lines within the found file. It then calls `SourceFile::line_begin_pos`, which redoes the binary search over the lines within the found file. This commit removes the second binary search over the lines, instead getting the line starting pos directly using the result of the first binary search over the lines. (And likewise for `get_span_loc`, in the cranelift backend.) --- compiler/rustc_codegen_cranelift/src/debuginfo/line_info.rs | 2 +- compiler/rustc_codegen_llvm/src/debuginfo/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/debuginfo/line_info.rs b/compiler/rustc_codegen_cranelift/src/debuginfo/line_info.rs index 463de6a91c74c..1b454b6667c23 100644 --- a/compiler/rustc_codegen_cranelift/src/debuginfo/line_info.rs +++ b/compiler/rustc_codegen_cranelift/src/debuginfo/line_info.rs @@ -81,7 +81,7 @@ impl DebugContext { match tcx.sess.source_map().lookup_line(span.lo()) { Ok(SourceFileAndLine { sf: file, line }) => { - let line_pos = file.line_begin_pos(span.lo()); + let line_pos = file.lines(|lines| lines[line]); ( file, diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs index aa7ae9355bcfa..44e591ceef18a 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs @@ -262,7 +262,7 @@ impl CodegenCx<'_, '_> { pub fn lookup_debug_loc(&self, pos: BytePos) -> DebugLoc { let (file, line, col) = match self.sess().source_map().lookup_line(pos) { Ok(SourceFileAndLine { sf: file, line }) => { - let line_pos = file.line_begin_pos(pos); + let line_pos = file.lines(|lines| lines[line]); // Use 1-based indexing. let line = (line + 1) as u32; From b4c6e19adeffad05e6039c21fdbda2097f3a2485 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 08:57:32 +1000 Subject: [PATCH 2/9] Replace a `lookup_debug_loc` call. `lookup_debug_loc` finds a file, line, and column, which requires two binary searches. But this call site only needs the file. This commit replaces the call with `lookup_source_file`, which does a single binary search. --- .../rustc_codegen_llvm/src/debuginfo/create_scope_map.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/create_scope_map.rs b/compiler/rustc_codegen_llvm/src/debuginfo/create_scope_map.rs index 64961baf272f4..65cbd5edc5927 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/create_scope_map.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/create_scope_map.rs @@ -65,10 +65,10 @@ fn make_mir_scope<'ll, 'tcx>( debug_context.scopes[parent] } else { // The root is the function itself. - let loc = cx.lookup_debug_loc(mir.span.lo()); + let file = cx.sess().source_map().lookup_source_file(mir.span.lo()); debug_context.scopes[scope] = DebugScope { - file_start_pos: loc.file.start_pos, - file_end_pos: loc.file.end_pos, + file_start_pos: file.start_pos, + file_end_pos: file.end_pos, ..debug_context.scopes[scope] }; instantiated.insert(scope); From 45fcd1d0c5df8287a3ffe620fa41b4ab478bc323 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 10:37:43 +1000 Subject: [PATCH 3/9] Use `partition_point` in `SourceMap::lookup_source_file_idx`. This makes it (a) a little simpler, and (b) more similar to `SourceFile::lookup_line`. --- compiler/rustc_span/src/source_map.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_span/src/source_map.rs b/compiler/rustc_span/src/source_map.rs index c53fe084c4db0..86716da1712fb 100644 --- a/compiler/rustc_span/src/source_map.rs +++ b/compiler/rustc_span/src/source_map.rs @@ -1072,11 +1072,7 @@ impl SourceMap { /// This index is guaranteed to be valid for the lifetime of this `SourceMap`, /// since `source_files` is a `MonotonicVec` pub fn lookup_source_file_idx(&self, pos: BytePos) -> usize { - self.files - .borrow() - .source_files - .binary_search_by_key(&pos, |key| key.start_pos) - .unwrap_or_else(|p| p - 1) + self.files.borrow().source_files.partition_point(|x| x.start_pos <= pos) - 1 } pub fn count_lines(&self) -> usize { From de1914af342bbd1f20388f52d6ea342420de6fc0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 11:30:27 +1000 Subject: [PATCH 4/9] Avoid an unnecessary use of `SmallStr`. I don't know why `SmallStr` was used here; some ad hoc profiling showed this code is not that hot, the string is usually empty, and when it's not empty it's usually very short. However, the use of a `SmallStr<1024>` does result in 1024 byte `memcpy` call on each execution, which shows up when I do `memcpy` profiling. So using a normal string makes the code both simpler and very slightly faster. --- compiler/rustc_codegen_llvm/src/attributes.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/attributes.rs b/compiler/rustc_codegen_llvm/src/attributes.rs index 6d00464e0a0b3..39275272e4226 100644 --- a/compiler/rustc_codegen_llvm/src/attributes.rs +++ b/compiler/rustc_codegen_llvm/src/attributes.rs @@ -1,7 +1,6 @@ //! Set and unset common attributes on LLVM values. use rustc_codegen_ssa::traits::*; -use rustc_data_structures::small_str::SmallStr; use rustc_hir::def_id::DefId; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::ty::{self, TyCtxt}; @@ -481,8 +480,8 @@ pub fn from_fn_attrs<'ll, 'tcx>( let global_features = cx.tcx.global_backend_features(()).iter().map(|s| s.as_str()); let function_features = function_features.iter().map(|s| s.as_str()); - let target_features = - global_features.chain(function_features).intersperse(",").collect::>(); + let target_features: String = + global_features.chain(function_features).intersperse(",").collect(); if !target_features.is_empty() { to_add.push(llvm::CreateAttrStringValue(cx.llcx, "target-features", &target_features)); } From f2d863fa75019791042722f8f9f58dbb13894d0a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 29 Jun 2023 11:42:54 +1000 Subject: [PATCH 5/9] Remove `SmallStr`. It no longer has any uses. If it's needed in the future, it can be easily reinstated. Or a crate such as `smallstr` can be used, much like we use `smallvec`. --- compiler/rustc_data_structures/src/lib.rs | 1 - .../rustc_data_structures/src/small_str.rs | 68 ------------------- .../src/small_str/tests.rs | 20 ------ 3 files changed, 89 deletions(-) delete mode 100644 compiler/rustc_data_structures/src/small_str.rs delete mode 100644 compiler/rustc_data_structures/src/small_str/tests.rs diff --git a/compiler/rustc_data_structures/src/lib.rs b/compiler/rustc_data_structures/src/lib.rs index 859e384d8b529..3deb9c5c2f5ce 100644 --- a/compiler/rustc_data_structures/src/lib.rs +++ b/compiler/rustc_data_structures/src/lib.rs @@ -68,7 +68,6 @@ pub mod macros; pub mod obligation_forest; pub mod sip128; pub mod small_c_str; -pub mod small_str; pub mod snapshot_map; pub mod svh; pub use ena::snapshot_vec; diff --git a/compiler/rustc_data_structures/src/small_str.rs b/compiler/rustc_data_structures/src/small_str.rs deleted file mode 100644 index 800acb1b03e5a..0000000000000 --- a/compiler/rustc_data_structures/src/small_str.rs +++ /dev/null @@ -1,68 +0,0 @@ -use smallvec::SmallVec; - -#[cfg(test)] -mod tests; - -/// Like SmallVec but for strings. -#[derive(Default)] -pub struct SmallStr(SmallVec<[u8; N]>); - -impl SmallStr { - #[inline] - pub fn new() -> Self { - SmallStr(SmallVec::default()) - } - - #[inline] - pub fn push_str(&mut self, s: &str) { - self.0.extend_from_slice(s.as_bytes()); - } - - #[inline] - pub fn empty(&self) -> bool { - self.0.is_empty() - } - - #[inline] - pub fn spilled(&self) -> bool { - self.0.spilled() - } - - #[inline] - pub fn as_str(&self) -> &str { - unsafe { std::str::from_utf8_unchecked(self.0.as_slice()) } - } -} - -impl std::ops::Deref for SmallStr { - type Target = str; - - #[inline] - fn deref(&self) -> &str { - self.as_str() - } -} - -impl> FromIterator for SmallStr { - #[inline] - fn from_iter(iter: T) -> Self - where - T: IntoIterator, - { - let mut s = SmallStr::default(); - s.extend(iter); - s - } -} - -impl> Extend for SmallStr { - #[inline] - fn extend(&mut self, iter: T) - where - T: IntoIterator, - { - for a in iter.into_iter() { - self.push_str(a.as_ref()); - } - } -} diff --git a/compiler/rustc_data_structures/src/small_str/tests.rs b/compiler/rustc_data_structures/src/small_str/tests.rs deleted file mode 100644 index 7635a9b7204db..0000000000000 --- a/compiler/rustc_data_structures/src/small_str/tests.rs +++ /dev/null @@ -1,20 +0,0 @@ -use super::*; - -#[test] -fn empty() { - let s = SmallStr::<1>::new(); - assert!(s.empty()); - assert_eq!("", s.as_str()); - assert!(!s.spilled()); -} - -#[test] -fn from_iter() { - let s = ["aa", "bb", "cc"].iter().collect::>(); - assert_eq!("aabbcc", s.as_str()); - assert!(!s.spilled()); - - let s = ["aa", "bb", "cc", "dd"].iter().collect::>(); - assert_eq!("aabbccdd", s.as_str()); - assert!(s.spilled()); -} From d20b1a8f6ba8ea753726fd70ce0e525f3bccffb9 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 13:56:53 +1000 Subject: [PATCH 6/9] Set capacity of the string passed to `push_item_name`. Other callsites already do this, but these two were missed. This avoids some allocations. --- compiler/rustc_codegen_llvm/src/debuginfo/mod.rs | 2 +- compiler/rustc_codegen_llvm/src/debuginfo/namespace.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs index 44e591ceef18a..c2f16cad3fcb1 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs @@ -331,7 +331,7 @@ impl<'ll, 'tcx> DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> { llvm::LLVMRustDIBuilderCreateSubroutineType(DIB(self), fn_signature) }; - let mut name = String::new(); + let mut name = String::with_capacity(64); type_names::push_item_name(tcx, def_id, false, &mut name); // Find the enclosing function, in case this is a closure. diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/namespace.rs b/compiler/rustc_codegen_llvm/src/debuginfo/namespace.rs index d5ea48c311b94..fa61c7dde18b4 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/namespace.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/namespace.rs @@ -28,7 +28,7 @@ pub fn item_namespace<'ll>(cx: &CodegenCx<'ll, '_>, def_id: DefId) -> &'ll DISco .map(|parent| item_namespace(cx, DefId { krate: def_id.krate, index: parent })); let namespace_name_string = { - let mut output = String::new(); + let mut output = String::with_capacity(64); type_names::push_item_name(cx.tcx, def_id, false, &mut output); output }; From 81436ebd55e3512282bb3954e8fb2a936f1ef495 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 14:07:43 +1000 Subject: [PATCH 7/9] Use `SmallVec` for the `bundles` vectors. They never have a length of more than two. So this commit changes them to `SmallVec<[_; 2]>`. Also, we possibly push `None` values and then filter those `None` values out again with `retain`. So this commit removes the `retain` and instead only pushes the values if they are `Some(_)`. --- compiler/rustc_codegen_llvm/src/builder.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index b4aa001547c4c..2a4bb1709df76 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -23,6 +23,7 @@ use rustc_span::Span; use rustc_symbol_mangling::typeid::{kcfi_typeid_for_fnabi, typeid_for_fnabi, TypeIdOptions}; use rustc_target::abi::{self, call::FnAbi, Align, Size, WrappingRange}; use rustc_target::spec::{HasTargetSpec, SanitizerSet, Target}; +use smallvec::SmallVec; use std::borrow::Cow; use std::iter; use std::ops::Deref; @@ -225,7 +226,10 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let args = self.check_call("invoke", llty, llfn, args); let funclet_bundle = funclet.map(|funclet| funclet.bundle()); let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw); - let mut bundles = vec![funclet_bundle]; + let mut bundles: SmallVec<[_; 2]> = SmallVec::new(); + if funclet_bundle.is_some() { + bundles.push(funclet_bundle); + } // Emit CFI pointer type membership test self.cfi_type_test(fn_attrs, fn_abi, llfn); @@ -233,9 +237,10 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { // Emit KCFI operand bundle let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, llfn); let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw); - bundles.push(kcfi_bundle); + if kcfi_bundle.is_some() { + bundles.push(kcfi_bundle); + } - bundles.retain(|bundle| bundle.is_some()); let invoke = unsafe { llvm::LLVMRustBuildInvoke( self.llbuilder, @@ -1189,7 +1194,10 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let args = self.check_call("call", llty, llfn, args); let funclet_bundle = funclet.map(|funclet| funclet.bundle()); let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw); - let mut bundles = vec![funclet_bundle]; + let mut bundles: SmallVec<[_; 2]> = SmallVec::new(); + if funclet_bundle.is_some() { + bundles.push(funclet_bundle); + } // Emit CFI pointer type membership test self.cfi_type_test(fn_attrs, fn_abi, llfn); @@ -1197,9 +1205,10 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { // Emit KCFI operand bundle let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, llfn); let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw); - bundles.push(kcfi_bundle); + if kcfi_bundle.is_some() { + bundles.push(kcfi_bundle); + } - bundles.retain(|bundle| bundle.is_some()); let call = unsafe { llvm::LLVMRustBuildCall( self.llbuilder, From 8d7084d65fc3c1956b0c1459d26cb5fa9b811525 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 14:09:51 +1000 Subject: [PATCH 8/9] Simplify the `bundles` vectors. After the last commit, they contain `Option<&OperandBundleDef<'a>>` but the values are always `Some(_)`. This commit removes the needless `Option` wrapper. This also simplifies the type signatures of `LLVMRustBuild{Invoke,Call}`, which were relying on the fact that the represention of `Option<&T>` is the same as `&T` for non-`None` values. --- compiler/rustc_codegen_llvm/src/builder.rs | 8 ++++---- compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 2a4bb1709df76..3df7a7c9c5ac7 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -227,7 +227,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let funclet_bundle = funclet.map(|funclet| funclet.bundle()); let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw); let mut bundles: SmallVec<[_; 2]> = SmallVec::new(); - if funclet_bundle.is_some() { + if let Some(funclet_bundle) = funclet_bundle { bundles.push(funclet_bundle); } @@ -237,7 +237,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { // Emit KCFI operand bundle let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, llfn); let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw); - if kcfi_bundle.is_some() { + if let Some(kcfi_bundle) = kcfi_bundle { bundles.push(kcfi_bundle); } @@ -1195,7 +1195,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let funclet_bundle = funclet.map(|funclet| funclet.bundle()); let funclet_bundle = funclet_bundle.as_ref().map(|b| &*b.raw); let mut bundles: SmallVec<[_; 2]> = SmallVec::new(); - if funclet_bundle.is_some() { + if let Some(funclet_bundle) = funclet_bundle { bundles.push(funclet_bundle); } @@ -1205,7 +1205,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { // Emit KCFI operand bundle let kcfi_bundle = self.kcfi_operand_bundle(fn_attrs, fn_abi, llfn); let kcfi_bundle = kcfi_bundle.as_ref().map(|b| &*b.raw); - if kcfi_bundle.is_some() { + if let Some(kcfi_bundle) = kcfi_bundle { bundles.push(kcfi_bundle); } diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 6ef3418cc5f77..fdc5f3b193e37 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1301,7 +1301,7 @@ extern "C" { NumArgs: c_uint, Then: &'a BasicBlock, Catch: &'a BasicBlock, - OpBundles: *const Option<&OperandBundleDef<'a>>, + OpBundles: *const &OperandBundleDef<'a>, NumOpBundles: c_uint, Name: *const c_char, ) -> &'a Value; @@ -1673,7 +1673,7 @@ extern "C" { Fn: &'a Value, Args: *const &'a Value, NumArgs: c_uint, - OpBundles: *const Option<&OperandBundleDef<'a>>, + OpBundles: *const &OperandBundleDef<'a>, NumOpBundles: c_uint, ) -> &'a Value; pub fn LLVMRustBuildMemCpy<'a>( From 7e786e81b00cf48a664084d30d4f82f408825397 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 28 Jun 2023 15:21:10 +1000 Subject: [PATCH 9/9] Avoid cloning `LocalDecls`. `DerefChecker` can just hold a reference instead. This avoids quite a lot of allocations for some benchmarks. --- compiler/rustc_mir_transform/src/deref_separator.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir_transform/src/deref_separator.rs b/compiler/rustc_mir_transform/src/deref_separator.rs index a39026751a783..95898b5b73cdb 100644 --- a/compiler/rustc_mir_transform/src/deref_separator.rs +++ b/compiler/rustc_mir_transform/src/deref_separator.rs @@ -8,13 +8,13 @@ use rustc_middle::ty::TyCtxt; pub struct Derefer; -pub struct DerefChecker<'tcx> { +pub struct DerefChecker<'a, 'tcx> { tcx: TyCtxt<'tcx>, patcher: MirPatch<'tcx>, - local_decls: IndexVec>, + local_decls: &'a IndexVec>, } -impl<'tcx> MutVisitor<'tcx> for DerefChecker<'tcx> { +impl<'a, 'tcx> MutVisitor<'tcx> for DerefChecker<'a, 'tcx> { fn tcx(&self) -> TyCtxt<'tcx> { self.tcx } @@ -36,7 +36,7 @@ impl<'tcx> MutVisitor<'tcx> for DerefChecker<'tcx> { for (idx, (p_ref, p_elem)) in place.iter_projections().enumerate() { if !p_ref.projection.is_empty() && p_elem == ProjectionElem::Deref { - let ty = p_ref.ty(&self.local_decls, self.tcx).ty; + let ty = p_ref.ty(self.local_decls, self.tcx).ty; let temp = self.patcher.new_internal_with_info( ty, self.local_decls[p_ref.local].source_info.span, @@ -70,7 +70,7 @@ impl<'tcx> MutVisitor<'tcx> for DerefChecker<'tcx> { pub fn deref_finder<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let patch = MirPatch::new(body); - let mut checker = DerefChecker { tcx, patcher: patch, local_decls: body.local_decls.clone() }; + let mut checker = DerefChecker { tcx, patcher: patch, local_decls: &body.local_decls }; for (bb, data) in body.basic_blocks.as_mut_preserves_cfg().iter_enumerated_mut() { checker.visit_basic_block_data(bb, data);