unify two -Ctarget-feature parsers

RalfJung · RalfJung · commit 3d8a3cb93edd · 2025-05-23T08:31:33.000+02:00
This does change the logic a bit: previously, we didn't forward reverse
implications of negated features to the backend, instead relying on the backend
to handle the implication itself.
diff --git a/compiler/rustc_middle/src/target_features.rs b/compiler/rustc_middle/src/target_features.rs
@@ -158,6 +158,58 @@ pub fn check_target_feature_trait_unsafe(tcx: TyCtxt<'_>, id: LocalDefId, attr_s
     }
 }
 
+/// Parse the value of `-Ctarget-feature`, also expanding implied features,
+/// and call the closure for each (expanded) Rust feature. If the list contains
+/// a syntactically invalid item (not starting with `+`/`-`), the error callback is invoked.
+fn parse_rust_feature_flag<'a>(
+    sess: &Session,
+    target_feature_flag: &'a str,
+    err_callback: impl Fn(&'a str),
+    mut callback: impl FnMut(
+        /* base_feature */ &'a str,
+        /* with_implied */ FxHashSet<&'a str>,
+        /* enable */ bool,
+    ),
+) {
+    // A cache for the backwards implication map.
+    let mut inverse_implied_features: Option<FxHashMap<&str, FxHashSet<&str>>> = None;
+
+    for feature in target_feature_flag.split(',') {
+        if let Some(base_feature) = feature.strip_prefix('+') {
+            callback(base_feature, sess.target.implied_target_features(base_feature), true)
+        } else if let Some(base_feature) = feature.strip_prefix('-') {
+            // If `f1` implies `f2`, then `!f2` implies `!f1` -- this is standard logical contraposition.
+            // So we have to find all the reverse implications of `base_feature` and disable them, too.
+
+            let inverse_implied_features = inverse_implied_features.get_or_insert_with(|| {
+                let mut set: FxHashMap<&str, FxHashSet<&str>> = FxHashMap::default();
+                for (f, _, is) in sess.target.rust_target_features() {
+                    for i in is.iter() {
+                        set.entry(i).or_default().insert(f);
+                    }
+                }
+                set
+            });
+
+            // Inverse mplied target features have their own inverse 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 = vec![base_feature];
+            while let Some(new_feature) = new_features.pop() {
+                if features.insert(new_feature) {
+                    if let Some(implied_features) = inverse_implied_features.get(&new_feature) {
+                        new_features.extend(implied_features)
+                    }
+                }
+            }
+
+            callback(base_feature, features, false)
+        } else if !feature.is_empty() {
+            err_callback(feature)
+        }
+    }
+}
+
 /// Utility function for a codegen backend to compute `cfg(target_feature)`, or more specifically,
 /// to populate `sess.unstable_target_features` and `sess.target_features` (these are the first and
 /// 2nd component of the return value, respectively).
@@ -174,7 +226,7 @@ pub fn cfg(
 ) -> (Vec<Symbol>, Vec<Symbol>) {
     // 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.
-    let mut features: FxHashSet<Symbol> = sess
+    let mut features: UnordSet<Symbol> = sess
         .target
         .rust_target_features()
         .iter()
@@ -189,43 +241,23 @@ pub fn cfg(
         .collect();
 
     // Add enabled and remove disabled features.
-    for (enabled, feature) in
-        target_feature_flag.split(',').filter_map(|s| match s.chars().next() {
-            Some('+') => Some((true, Symbol::intern(&s[1..]))),
-            Some('-') => Some((false, Symbol::intern(&s[1..]))),
-            _ => None,
-        })
-    {
-        if enabled {
-            // Also add all transitively implied features.
-
-            // We don't care about the order in `features` since the only thing we use it for is the
-            // `features.contains` below.
+    parse_rust_feature_flag(
+        sess,
+        target_feature_flag,
+        /* err_callback */ |_| {},
+        |_base_feature, new_features, enabled| {
+            // Iteration order is irrelevant since this only influences an `UnordSet`.
             #[allow(rustc::potential_query_instability)]
-            features.extend(
-                sess.target
-                    .implied_target_features(feature.as_str())
-                    .iter()
-                    .map(|s| Symbol::intern(s)),
-            );
-        } else {
-            // Remove transitively reverse-implied features.
-
-            // We don't care about the order in `features` since the only thing we use it for is the
-            // `features.contains` below.
-            #[allow(rustc::potential_query_instability)]
-            features.retain(|f| {
-                if sess.target.implied_target_features(f.as_str()).contains(&feature.as_str()) {
-                    // If `f` if implies `feature`, then `!feature` implies `!f`, so we have to
-                    // remove `f`. (This is the standard logical contraposition principle.)
-                    false
-                } else {
-                    // We can keep `f`.
-                    true
+            if enabled {
+                features.extend(new_features.into_iter().map(|f| Symbol::intern(f)));
+            } else {
+                // Remove `new_features` from `features`.
+                for new in new_features {
+                    features.remove(&Symbol::intern(new));
                 }
-            });
-        }
-    }
+            }
+        },
+    );
 
     // Filter enabled features based on feature gates.
     let f = |allow_unstable| {
@@ -288,85 +320,81 @@ pub fn flag_to_backend_features<'a, const N: usize>(
 
     // Compute implied features
     let mut rust_features = vec![];
-    for feature in sess.opts.cg.target_feature.split(',') {
-        if let Some(feature) = feature.strip_prefix('+') {
-            rust_features.extend(
-                UnordSet::from(sess.target.implied_target_features(feature))
-                    .to_sorted_stable_ord()
-                    .iter()
-                    .map(|&&s| (true, s)),
-            )
-        } else if let Some(feature) = feature.strip_prefix('-') {
-            // FIXME: Why do we not remove implied features on "-" here?
-            // We do the equivalent above in `target_config`.
-            // See <https://github.com/rust-lang/rust/issues/134792>.
-            rust_features.push((false, feature));
-        } else if !feature.is_empty() {
+    parse_rust_feature_flag(
+        sess,
+        &sess.opts.cg.target_feature,
+        /* err_callback */
+        |feature| {
             if diagnostics {
                 sess.dcx().emit_warn(error::UnknownCTargetFeaturePrefix { feature });
             }
-        }
-    }
-    // Remove features that are meant for rustc, not the backend.
-    rust_features.retain(|(_, feature)| {
-        // Retain if it is not a rustc feature
-        !RUSTC_SPECIFIC_FEATURES.contains(feature)
-    });
-
-    // Check feature validity.
-    if diagnostics {
-        let mut featsmap = FxHashMap::default();
+        },
+        |base_feature, new_features, enable| {
+            // Skip features that are meant for rustc, not the backend.
+            if RUSTC_SPECIFIC_FEATURES.contains(&base_feature) {
+                return;
+            }
 
-        for &(enable, feature) in &rust_features {
-            let feature_state = known_features.iter().find(|&&(v, _, _)| v == feature);
-            match feature_state {
-                None => {
-                    // This is definitely not a valid Rust feature name. Maybe it is a backend feature name?
-                    // If so, give a better error message.
-                    let rust_feature = known_features.iter().find_map(|&(rust_feature, _, _)| {
-                        let backend_features = to_backend_features(rust_feature);
-                        if backend_features.contains(&feature)
-                            && !backend_features.contains(&rust_feature)
-                        {
-                            Some(rust_feature)
+            rust_features.extend(
+                UnordSet::from(new_features).to_sorted_stable_ord().iter().map(|&&s| (enable, s)),
+            );
+            // Check feature validity.
+            if diagnostics {
+                let feature_state = known_features.iter().find(|&&(v, _, _)| v == base_feature);
+                match feature_state {
+                    None => {
+                        // This is definitely not a valid Rust feature name. Maybe it is a backend feature name?
+                        // If so, give a better error message.
+                        let rust_feature =
+                            known_features.iter().find_map(|&(rust_feature, _, _)| {
+                                let backend_features = to_backend_features(rust_feature);
+                                if backend_features.contains(&base_feature)
+                                    && !backend_features.contains(&rust_feature)
+                                {
+                                    Some(rust_feature)
+                                } else {
+                                    None
+                                }
+                            });
+                        let unknown_feature = if let Some(rust_feature) = rust_feature {
+                            error::UnknownCTargetFeature {
+                                feature: base_feature,
+                                rust_feature: error::PossibleFeature::Some { rust_feature },
+                            }
                         } else {
-                            None
-                        }
-                    });
-                    let unknown_feature = if let Some(rust_feature) = rust_feature {
-                        error::UnknownCTargetFeature {
-                            feature,
-                            rust_feature: error::PossibleFeature::Some { rust_feature },
-                        }
-                    } else {
-                        error::UnknownCTargetFeature {
-                            feature,
-                            rust_feature: error::PossibleFeature::None,
+                            error::UnknownCTargetFeature {
+                                feature: base_feature,
+                                rust_feature: error::PossibleFeature::None,
+                            }
+                        };
+                        sess.dcx().emit_warn(unknown_feature);
+                    }
+                    Some((_, stability, _)) => {
+                        if let Err(reason) = stability.toggle_allowed() {
+                            sess.dcx().emit_warn(error::ForbiddenCTargetFeature {
+                                feature: base_feature,
+                                enabled: if enable { "enabled" } else { "disabled" },
+                                reason,
+                            });
+                        } else if stability.requires_nightly().is_some() {
+                            // An unstable feature. Warn about using it. It makes little sense
+                            // to hard-error here since we just warn about fully unknown
+                            // features above.
+                            sess.dcx()
+                                .emit_warn(error::UnstableCTargetFeature { feature: base_feature });
                         }
-                    };
-                    sess.dcx().emit_warn(unknown_feature);
-                }
-                Some((_, stability, _)) => {
-                    if let Err(reason) = stability.toggle_allowed() {
-                        sess.dcx().emit_warn(error::ForbiddenCTargetFeature {
-                            feature,
-                            enabled: if enable { "enabled" } else { "disabled" },
-                            reason,
-                        });
-                    } else if stability.requires_nightly().is_some() {
-                        // An unstable feature. Warn about using it. It makes little sense
-                        // to hard-error here since we just warn about fully unknown
-                        // features above.
-                        sess.dcx().emit_warn(error::UnstableCTargetFeature { feature });
                     }
                 }
             }
+        },
+    );
 
-            // FIXME(nagisa): figure out how to not allocate a full hashset here.
-            featsmap.insert(feature, enable);
-        }
-
-        if let Some(f) = check_tied_features(sess, &featsmap) {
+    if diagnostics {
+        // FIXME(nagisa): figure out how to not allocate a full hashmap here.
+        if let Some(f) = check_tied_features(
+            sess,
+            &FxHashMap::from_iter(rust_features.iter().map(|&(enable, feature)| (feature, enable))),
+        ) {
             sess.dcx().emit_err(error::TargetFeatureDisableOrEnable {
                 features: f,
                 span: None,
diff --git a/tests/codegen/target-feature-negative-implication.rs b/tests/codegen/target-feature-negative-implication.rs
@@ -0,0 +1,20 @@
+//@ add-core-stubs
+//@ needs-llvm-components: x86
+//@ compile-flags: --target=x86_64-unknown-linux-gnu
+//@ compile-flags: -Ctarget-feature=-avx2
+
+#![feature(no_core, lang_items)]
+#![crate_type = "lib"]
+#![no_core]
+
+extern crate minicore;
+use minicore::*;
+
+#[no_mangle]
+pub unsafe fn banana() {
+    // CHECK-LABEL: @banana()
+    // CHECK-SAME: [[BANANAATTRS:#[0-9]+]] {
+}
+
+// CHECK: attributes [[BANANAATTRS]]
+// CHECK-SAME: -avx512
diff --git a/tests/codegen/target-feature-overrides.rs b/tests/codegen/target-feature-overrides.rs
@@ -1,3 +1,4 @@
+// ignore-tidy-linelength
 //@ add-core-stubs
 //@ revisions: COMPAT INCOMPAT
 //@ needs-llvm-components: x86
@@ -39,7 +40,7 @@ pub unsafe fn banana() -> u32 {
 
 // CHECK: attributes [[APPLEATTRS]]
 // COMPAT-SAME: "target-features"="+avx,+avx2,{{.*}}"
-// INCOMPAT-SAME: "target-features"="-avx2,-avx,+avx,{{.*}}"
+// INCOMPAT-SAME: "target-features"="{{(-[^,]+,)*}}-avx2{{(,-[^,]+)*}},-avx{{(,-[^,]+)*}},+avx{{(,\+[^,]+)*}}"
 // CHECK: attributes [[BANANAATTRS]]
 // COMPAT-SAME: "target-features"="+avx,+avx2,{{.*}}"
-// INCOMPAT-SAME: "target-features"="-avx2,-avx"
+// INCOMPAT-SAME: "target-features"="{{(-[^,]+,)*}}-avx2{{(,-[^,]+)*}},-avx{{(,-[^,]+)*}}"
diff --git a/tests/codegen/tied-features-strength.rs b/tests/codegen/tied-features-strength.rs
@@ -4,14 +4,23 @@
 //@ compile-flags: --crate-type=rlib --target=aarch64-unknown-linux-gnu
 //@ needs-llvm-components: aarch64
 
+// Rust made SVE require neon.
 //@ [ENABLE_SVE] compile-flags: -C target-feature=+sve -Copt-level=0
-// ENABLE_SVE: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+sve,?)|(\+neon,?)|(\+fp-armv8,?))*}}" }
+// ENABLE_SVE: attributes #0
+// ENABLE_SVE-SAME: +neon
+// ENABLE_SVE-SAME: +sve
 
+// However, disabling SVE does not disable neon.
 //@ [DISABLE_SVE] compile-flags: -C target-feature=-sve -Copt-level=0
-// DISABLE_SVE: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(-sve,?)|(\+neon,?))*}}" }
+// DISABLE_SVE: attributes #0
+// DISABLE_SVE-NOT: -neon
+// DISABLE_SVE-SAME: -sve
 
+// OTOH, neon fn `fp-armv8` are fully tied; toggling neon must toggle `fp-armv8` the same way.
 //@ [DISABLE_NEON] compile-flags: -C target-feature=-neon -Copt-level=0
-// DISABLE_NEON: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(-fp-armv8,?)|(-neon,?))*}}" }
+// DISABLE_NEON: attributes #0
+// DISABLE_NEON-SAME: -neon
+// DISABLE_NEON-SAME: -fp-armv8
 
 //@ [ENABLE_NEON] compile-flags: -C target-feature=+neon -Copt-level=0
 // ENABLE_NEON: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fp-armv8,?)|(\+neon,?))*}}" }
diff --git a/tests/ui/target-feature/abi-incompatible-target-feature-flag-enable.riscv.stderr b/tests/ui/target-feature/abi-incompatible-target-feature-flag-enable.riscv.stderr
@@ -7,13 +7,5 @@ warning: unstable feature specified for `-Ctarget-feature`: `d`
    |
    = note: this feature is not stably supported; its behavior can change in the future
 
-warning: unstable feature specified for `-Ctarget-feature`: `f`
-   |
-   = note: this feature is not stably supported; its behavior can change in the future
-
-warning: unstable feature specified for `-Ctarget-feature`: `zicsr`
-   |
-   = note: this feature is not stably supported; its behavior can change in the future
-
-warning: 4 warnings emitted
+warning: 2 warnings emitted
 
diff --git a/tests/ui/target-feature/abi-incompatible-target-feature-flag-enable.rs b/tests/ui/target-feature/abi-incompatible-target-feature-flag-enable.rs
@@ -18,5 +18,3 @@ pub trait Freeze {}
 
 //~? WARN must be disabled to ensure that the ABI of the current target can be implemented correctly
 //~? WARN unstable feature specified for `-Ctarget-feature`
-//[riscv]~? WARN unstable feature specified for `-Ctarget-feature`
-//[riscv]~? WARN unstable feature specified for `-Ctarget-feature`
