From 1df93fd6a7a164ebd8b7868d9382e3fb37f4642e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 25 Feb 2025 13:58:31 +1100 Subject: [PATCH 1/5] Avoid double interning of feature names. Also improve some comments. --- compiler/rustc_codegen_llvm/src/llvm_util.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index 5cc4f4ab9e6..23e8a986564 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -313,11 +313,11 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec Vec Some((true, Symbol::intern(&s[1..]))), @@ -398,13 +398,12 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec Date: Tue, 25 Feb 2025 14:13:16 +1100 Subject: [PATCH 2/5] Simplify `implied_target_features`. Currently its argument is an iterator, but in practice it's always a singleton. --- compiler/rustc_codegen_gcc/src/gcc_util.rs | 2 +- compiler/rustc_codegen_llvm/src/llvm_util.rs | 6 +++--- compiler/rustc_codegen_ssa/src/target_features.rs | 2 +- compiler/rustc_target/src/target_features.rs | 12 +++++------- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/gcc_util.rs b/compiler/rustc_codegen_gcc/src/gcc_util.rs index 4e8c8aaaf5c..6eae0c24f48 100644 --- a/compiler/rustc_codegen_gcc/src/gcc_util.rs +++ b/compiler/rustc_codegen_gcc/src/gcc_util.rs @@ -48,7 +48,7 @@ pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec Vec Vec( - &self, - base_features: impl Iterator, - ) -> FxHashSet<&'a str> { + // Note: the returned set includes `base_feature`. + pub fn implied_target_features<'a>(&self, base_feature: &'a str) -> FxHashSet<&'a str> { let implied_features = self.rust_target_features().iter().map(|(f, _, i)| (f, i)).collect::>(); - // implied target features have their own implied target features, so we traverse the - // map until there are no more features to add + // Implied target features have their own implied target features, so we traverse the + // map until there are no more features to add. let mut features = FxHashSet::default(); - let mut new_features = base_features.collect::>(); + let mut new_features = vec![base_feature]; while let Some(new_feature) = new_features.pop() { if features.insert(new_feature) { if let Some(implied_features) = implied_features.get(&new_feature) { From 936a8232df56cf6fbf41beb3b2329a99498d2167 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 25 Feb 2025 15:25:54 +1100 Subject: [PATCH 3/5] Change signature of `target_features_cfg`. Currently it is called twice, once with `allow_unstable` set to true and once with it set to false. This results in some duplicated work. Most notably, for the LLVM backend, `LLVMRustHasFeature` is called twice for every feature, and it's moderately slow. For very short running compilations on platforms with many features (e.g. a `check` build of hello-world on x86) this is a significant fraction of runtime. This commit changes `target_features_cfg` so it is only called once, and it now returns a pair of feature sets. This halves the number of `LLVMRustHasFeature` calls. --- compiler/rustc_codegen_cranelift/src/lib.rs | 13 ++-- compiler/rustc_codegen_gcc/src/lib.rs | 68 ++++++++++--------- compiler/rustc_codegen_llvm/src/lib.rs | 4 +- compiler/rustc_codegen_llvm/src/llvm_util.rs | 54 ++++++++------- .../rustc_codegen_ssa/src/traits/backend.rs | 9 ++- compiler/rustc_interface/src/util.rs | 8 +-- 6 files changed, 85 insertions(+), 71 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/lib.rs b/compiler/rustc_codegen_cranelift/src/lib.rs index a3f43744875..06939beb374 100644 --- a/compiler/rustc_codegen_cranelift/src/lib.rs +++ b/compiler/rustc_codegen_cranelift/src/lib.rs @@ -176,13 +176,9 @@ impl CodegenBackend for CraneliftCodegenBackend { } } - fn target_features_cfg( - &self, - sess: &Session, - _allow_unstable: bool, - ) -> Vec { + fn target_features_cfg(&self, sess: &Session) -> (Vec, Vec) { // FIXME return the actually used target features. this is necessary for #[cfg(target_feature)] - if sess.target.arch == "x86_64" && sess.target.os != "none" { + let target_features = if sess.target.arch == "x86_64" && sess.target.os != "none" { // x86_64 mandates SSE2 support and rustc requires the x87 feature to be enabled vec![sym::fsxr, sym::sse, sym::sse2, Symbol::intern("x87")] } else if sess.target.arch == "aarch64" { @@ -196,7 +192,10 @@ impl CodegenBackend for CraneliftCodegenBackend { } } else { vec![] - } + }; + // FIXME do `unstable_target_features` properly + let unstable_target_features = target_features.clone(); + (target_features, unstable_target_features) } fn print_version(&self) { diff --git a/compiler/rustc_codegen_gcc/src/lib.rs b/compiler/rustc_codegen_gcc/src/lib.rs index f090597f953..d478b2af46c 100644 --- a/compiler/rustc_codegen_gcc/src/lib.rs +++ b/compiler/rustc_codegen_gcc/src/lib.rs @@ -259,8 +259,8 @@ impl CodegenBackend for GccCodegenBackend { .join(sess) } - fn target_features_cfg(&self, sess: &Session, allow_unstable: bool) -> Vec { - target_features_cfg(sess, allow_unstable, &self.target_info) + fn target_features_cfg(&self, sess: &Session) -> (Vec, Vec) { + target_features_cfg(sess, &self.target_info) } } @@ -486,35 +486,41 @@ fn to_gcc_opt_level(optlevel: Option) -> OptimizationLevel { /// Returns the features that should be set in `cfg(target_feature)`. fn target_features_cfg( sess: &Session, - allow_unstable: bool, target_info: &LockedTargetInfo, -) -> Vec { +) -> (Vec, Vec) { // TODO(antoyo): use global_gcc_features. - sess.target - .rust_target_features() - .iter() - .filter_map(|&(feature, gate, _)| { - if allow_unstable - || (gate.in_cfg() && (sess.is_nightly_build() || gate.requires_nightly().is_none())) - { - Some(feature) - } else { - None - } - }) - .filter(|feature| { - // TODO: we disable Neon for now since we don't support the LLVM intrinsics for it. - if *feature == "neon" { - return false; - } - target_info.cpu_supports(feature) - /* - adx, aes, avx, avx2, avx512bf16, avx512bitalg, avx512bw, avx512cd, avx512dq, avx512er, avx512f, avx512fp16, avx512ifma, - avx512pf, avx512vbmi, avx512vbmi2, avx512vl, avx512vnni, avx512vp2intersect, avx512vpopcntdq, - bmi1, bmi2, cmpxchg16b, ermsb, f16c, fma, fxsr, gfni, lzcnt, movbe, pclmulqdq, popcnt, rdrand, rdseed, rtm, - sha, sse, sse2, sse3, sse4.1, sse4.2, sse4a, ssse3, tbm, vaes, vpclmulqdq, xsave, xsavec, xsaveopt, xsaves - */ - }) - .map(Symbol::intern) - .collect() + let f = |allow_unstable| { + sess.target + .rust_target_features() + .iter() + .filter_map(|&(feature, gate, _)| { + if allow_unstable + || (gate.in_cfg() + && (sess.is_nightly_build() || gate.requires_nightly().is_none())) + { + Some(feature) + } else { + None + } + }) + .filter(|feature| { + // TODO: we disable Neon for now since we don't support the LLVM intrinsics for it. + if *feature == "neon" { + return false; + } + target_info.cpu_supports(feature) + /* + adx, aes, avx, avx2, avx512bf16, avx512bitalg, avx512bw, avx512cd, avx512dq, avx512er, avx512f, avx512fp16, avx512ifma, + avx512pf, avx512vbmi, avx512vbmi2, avx512vl, avx512vnni, avx512vp2intersect, avx512vpopcntdq, + bmi1, bmi2, cmpxchg16b, ermsb, f16c, fma, fxsr, gfni, lzcnt, movbe, pclmulqdq, popcnt, rdrand, rdseed, rtm, + sha, sse, sse2, sse3, sse4.1, sse4.2, sse4a, ssse3, tbm, vaes, vpclmulqdq, xsave, xsavec, xsaveopt, xsaves + */ + }) + .map(Symbol::intern) + .collect() + }; + + let target_features = f(false); + let unstable_target_features = f(true); + (target_features, unstable_target_features) } diff --git a/compiler/rustc_codegen_llvm/src/lib.rs b/compiler/rustc_codegen_llvm/src/lib.rs index 8f72307eeba..833c8441196 100644 --- a/compiler/rustc_codegen_llvm/src/lib.rs +++ b/compiler/rustc_codegen_llvm/src/lib.rs @@ -338,8 +338,8 @@ impl CodegenBackend for LlvmCodegenBackend { llvm_util::print_version(); } - fn target_features_cfg(&self, sess: &Session, allow_unstable: bool) -> Vec { - target_features_cfg(sess, allow_unstable) + fn target_features_cfg(&self, sess: &Session) -> (Vec, Vec) { + target_features_cfg(sess) } fn codegen_crate<'tcx>( diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index f86ac13a87a..ed861a69b71 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -306,7 +306,7 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option Vec { +pub(crate) fn target_features_cfg(sess: &Session) -> (Vec, Vec) { let mut features: FxHashSet = Default::default(); // Add base features for the target. @@ -331,6 +331,9 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec Vec Vec Vec { - vec![] + fn target_features_cfg(&self, _sess: &Session) -> (Vec, Vec) { + (vec![], vec![]) } fn print_passes(&self) {} diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs index bc2aae7cd87..5cccab893bb 100644 --- a/compiler/rustc_interface/src/util.rs +++ b/compiler/rustc_interface/src/util.rs @@ -39,11 +39,11 @@ pub(crate) fn add_configuration( ) { let tf = sym::target_feature; - let unstable_target_features = codegen_backend.target_features_cfg(sess, true); - sess.unstable_target_features.extend(unstable_target_features.iter().cloned()); + let (target_features, unstable_target_features) = codegen_backend.target_features_cfg(sess); - let target_features = codegen_backend.target_features_cfg(sess, false); - sess.target_features.extend(target_features.iter().cloned()); + sess.unstable_target_features.extend(unstable_target_features.iter().copied()); + + sess.target_features.extend(target_features.iter().copied()); cfg.extend(target_features.into_iter().map(|feat| (tf, Some(feat)))); From 35b7994ea884aaa771b44fd89436f4feebf86c1b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 25 Feb 2025 16:27:09 +1100 Subject: [PATCH 4/5] Use `collect` to initialize `features`. --- compiler/rustc_codegen_llvm/src/llvm_util.rs | 53 +++++++++----------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index ed861a69b71..a4ada290ae1 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -307,8 +307,6 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option (Vec, Vec) { - let mut features: FxHashSet = Default::default(); - // Add base features for the target. // We do *not* add the -Ctarget-features there, and instead duplicate the logic for that below. // The reason is that if LLVM considers a feature implied but we do not, we don't want that to @@ -318,34 +316,33 @@ pub(crate) fn target_features_cfg(sess: &Session) -> (Vec, Vec) let target_machine = create_informational_target_machine(sess, true); // Compute which of the known target features are enabled in the 'base' target machine. We only // consider "supported" features; "forbidden" features are not reflected in `cfg` as of now. - features.extend( - sess.target - .rust_target_features() - .iter() - .filter(|(feature, _, _)| { - // skip checking special features, as LLVM may not understand them - if RUSTC_SPECIAL_FEATURES.contains(feature) { - return true; - } - // check that all features in a given smallvec are enabled - if let Some(feat) = to_llvm_features(sess, feature) { - for llvm_feature in feat { - let cstr = SmallCStr::new(llvm_feature); - // `LLVMRustHasFeature` is moderately expensive. On targets with many - // features (e.g. x86) these calls take a non-trivial fraction of runtime - // when compiling very small programs. - if !unsafe { llvm::LLVMRustHasFeature(target_machine.raw(), cstr.as_ptr()) } - { - return false; - } + let mut features: FxHashSet = sess + .target + .rust_target_features() + .iter() + .filter(|(feature, _, _)| { + // skip checking special features, as LLVM may not understand them + if RUSTC_SPECIAL_FEATURES.contains(feature) { + return true; + } + // check that all features in a given smallvec are enabled + if let Some(feat) = to_llvm_features(sess, feature) { + for llvm_feature in feat { + let cstr = SmallCStr::new(llvm_feature); + // `LLVMRustHasFeature` is moderately expensive. On targets with many + // features (e.g. x86) these calls take a non-trivial fraction of runtime + // when compiling very small programs. + if !unsafe { llvm::LLVMRustHasFeature(target_machine.raw(), cstr.as_ptr()) } { + return false; } - true - } else { - false } - }) - .map(|(feature, _, _)| Symbol::intern(feature)), - ); + true + } else { + false + } + }) + .map(|(feature, _, _)| Symbol::intern(feature)) + .collect(); // Add enabled and remove disabled features. for (enabled, feature) in From cee311454427bf5049f5493bb7c7d74df2abb369 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 25 Feb 2025 16:45:35 +1100 Subject: [PATCH 5/5] Remove out of date comment. No smallvecs here. --- compiler/rustc_codegen_llvm/src/llvm_util.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index a4ada290ae1..4a166b0872d 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -325,7 +325,6 @@ pub(crate) fn target_features_cfg(sess: &Session) -> (Vec, Vec) if RUSTC_SPECIAL_FEATURES.contains(feature) { return true; } - // check that all features in a given smallvec are enabled if let Some(feat) = to_llvm_features(sess, feature) { for llvm_feature in feat { let cstr = SmallCStr::new(llvm_feature);