Correctly handle target features

5 files changed, 289 insertions, 67 deletions

diff --git a/messages.ftl b/messages.ftl
index de9be3a5528..5ca0a2e1b6d 100644
--- a/messages.ftl
+++ b/messages.ftl
@@ -1,3 +1,7 @@
+codegen_gcc_unknown_ctarget_feature_prefix =
+    unknown feature specified for `-Ctarget-feature`: `{$feature}`
+    .note = features must begin with a `+` to enable or `-` to disable it
+
 codegen_gcc_invalid_minimum_alignment =
     invalid minimum global alignment: {$err}
 
@@ -23,3 +27,15 @@ codegen_gcc_lto_disallowed = lto can only be run for executables, cdylibs and st
 codegen_gcc_lto_dylib = lto cannot be used for `dylib` crate type without `-Zdylib-lto`
 
 codegen_gcc_lto_bitcode_from_rlib = failed to get bitcode from object file for LTO ({$gcc_err})
+
+codegen_gcc_unknown_ctarget_feature =
+    unknown feature specified for `-Ctarget-feature`: `{$feature}`
+    .note = it is still passed through to the codegen backend
+    .possible_feature = you might have meant: `{$rust_feature}`
+    .consider_filing_feature_request = consider filing a feature request
+
+codegen_gcc_missing_features =
+    add the missing features in a `target_feature` attribute
+
+codegen_gcc_target_feature_disable_or_enable =
+    the target features {$features} must all be either enabled or disabled together
diff --git a/src/attributes.rs b/src/attributes.rs
index 35682db9c78..ced13848c0b 100644
--- a/src/attributes.rs
+++ b/src/attributes.rs
@@ -4,72 +4,13 @@ use gccjit::Function;
 use rustc_attr::InstructionSetAttr;
 #[cfg(feature="master")]
 use rustc_attr::InlineAttr;
-use rustc_codegen_ssa::target_features::tied_target_features;
-use rustc_data_structures::fx::FxHashMap;
 use rustc_middle::ty;
 #[cfg(feature="master")]
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
-use rustc_session::Session;
 use rustc_span::symbol::sym;
-use smallvec::{smallvec, SmallVec};
 
 use crate::{context::CodegenCx, errors::TiedTargetFeatures};
-
-// Given a map from target_features to whether they are enabled or disabled,
-// ensure only valid combinations are allowed.
-pub fn check_tied_features(sess: &Session, features: &FxHashMap<&str, bool>) -> Option<&'static [&'static str]> {
-    for tied in tied_target_features(sess) {
-        // Tied features must be set to the same value, or not set at all
-        let mut tied_iter = tied.iter();
-        let enabled = features.get(tied_iter.next().unwrap());
-        if tied_iter.any(|feature| enabled != features.get(feature)) {
-            return Some(tied);
-        }
-    }
-    None
-}
-
-// TODO(antoyo): maybe move to a new module gcc_util.
-// To find a list of GCC's names, check https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
-fn to_gcc_features<'a>(sess: &Session, s: &'a str) -> SmallVec<[&'a str; 2]> {
-    let arch = if sess.target.arch == "x86_64" { "x86" } else { &*sess.target.arch };
-    match (arch, s) {
-        ("x86", "sse4.2") => smallvec!["sse4.2", "crc32"],
-        ("x86", "pclmulqdq") => smallvec!["pclmul"],
-        ("x86", "rdrand") => smallvec!["rdrnd"],
-        ("x86", "bmi1") => smallvec!["bmi"],
-        ("x86", "cmpxchg16b") => smallvec!["cx16"],
-        ("x86", "avx512vaes") => smallvec!["vaes"],
-        ("x86", "avx512gfni") => smallvec!["gfni"],
-        ("x86", "avx512vpclmulqdq") => smallvec!["vpclmulqdq"],
-        // NOTE: seems like GCC requires 'avx512bw' for 'avx512vbmi2'.
-        ("x86", "avx512vbmi2") => smallvec!["avx512vbmi2", "avx512bw"],
-        // NOTE: seems like GCC requires 'avx512bw' for 'avx512bitalg'.
-        ("x86", "avx512bitalg") => smallvec!["avx512bitalg", "avx512bw"],
-        ("aarch64", "rcpc2") => smallvec!["rcpc-immo"],
-        ("aarch64", "dpb") => smallvec!["ccpp"],
-        ("aarch64", "dpb2") => smallvec!["ccdp"],
-        ("aarch64", "frintts") => smallvec!["fptoint"],
-        ("aarch64", "fcma") => smallvec!["complxnum"],
-        ("aarch64", "pmuv3") => smallvec!["perfmon"],
-        ("aarch64", "paca") => smallvec!["pauth"],
-        ("aarch64", "pacg") => smallvec!["pauth"],
-        // Rust ties fp and neon together. In LLVM neon implicitly enables fp,
-        // but we manually enable neon when a feature only implicitly enables fp
-        ("aarch64", "f32mm") => smallvec!["f32mm", "neon"],
-        ("aarch64", "f64mm") => smallvec!["f64mm", "neon"],
-        ("aarch64", "fhm") => smallvec!["fp16fml", "neon"],
-        ("aarch64", "fp16") => smallvec!["fullfp16", "neon"],
-        ("aarch64", "jsconv") => smallvec!["jsconv", "neon"],
-        ("aarch64", "sve") => smallvec!["sve", "neon"],
-        ("aarch64", "sve2") => smallvec!["sve2", "neon"],
-        ("aarch64", "sve2-aes") => smallvec!["sve2-aes", "neon"],
-        ("aarch64", "sve2-sm4") => smallvec!["sve2-sm4", "neon"],
-        ("aarch64", "sve2-sha3") => smallvec!["sve2-sha3", "neon"],
-        ("aarch64", "sve2-bitperm") => smallvec!["sve2-bitperm", "neon"],
-        (_, s) => smallvec![s],
-    }
-}
+use crate::gcc_util::{check_tied_features, to_gcc_features};
 
 /// Get GCC attribute for the provided inline heuristic.
 #[cfg(feature="master")]
@@ -153,12 +94,31 @@ pub fn from_fn_attrs<'gcc, 'tcx>(
         }))
         .collect::<Vec<_>>();
 
-    // TODO(antoyo): check if we really need global backend features. (Maybe they could be applied
-    // globally?)
+    // TODO(antoyo): cg_llvm add global features to each function so that LTO keep them.
+    // Check if GCC requires the same.
     let mut global_features = cx.tcx.global_backend_features(()).iter().map(|s| s.as_str());
     function_features.extend(&mut global_features);
-    let target_features = function_features.join(",");
+    let target_features = function_features
+        .iter()
+        .filter_map(|feature| {
+            if feature.contains("soft-float") || feature.contains("retpoline-external-thunk") {
+                return None;
+            }
+
+            if feature.starts_with('-') {
+                Some(format!("no{}", feature))
+            }
+            else if feature.starts_with('+') {
+                Some(feature[1..].to_string())
+            }
+            else {
+                Some(feature.to_string())
+            }
+        })
+        .collect::<Vec<_>>()
+        .join(",");
     if !target_features.is_empty() {
+        println!("Function {:?}", function_features);
         #[cfg(feature="master")]
         func.add_attribute(FnAttribute::Target(&target_features));
     }
diff --git a/src/errors.rs b/src/errors.rs
index 19a967cb489..4bf3b71f503 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -1,8 +1,36 @@
-use rustc_errors::{DiagnosticArgValue, IntoDiagnosticArg};
-use rustc_macros::Diagnostic;
+use rustc_errors::{
+    DiagnosticArgValue, DiagnosticBuilder, ErrorGuaranteed, Handler, IntoDiagnostic, IntoDiagnosticArg,
+};
+use rustc_macros::{Diagnostic, Subdiagnostic};
 use rustc_span::Span;
 use std::borrow::Cow;
 
+use crate::fluent_generated as fluent;
+
+#[derive(Diagnostic)]
+#[diag(codegen_gcc_unknown_ctarget_feature_prefix)]
+#[note]
+pub(crate) struct UnknownCTargetFeaturePrefix<'a> {
+    pub feature: &'a str,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_gcc_unknown_ctarget_feature)]
+#[note]
+pub(crate) struct UnknownCTargetFeature<'a> {
+    pub feature: &'a str,
+    #[subdiagnostic]
+    pub rust_feature: PossibleFeature<'a>,
+}
+
+#[derive(Subdiagnostic)]
+pub(crate) enum PossibleFeature<'a> {
+    #[help(codegen_gcc_possible_feature)]
+    Some { rust_feature: &'a str },
+    #[help(codegen_gcc_consider_filing_feature_request)]
+    None,
+}
+
 struct ExitCode(Option<i32>);
 
 impl IntoDiagnosticArg for ExitCode {
@@ -71,3 +99,27 @@ pub(crate) struct LtoDylib;
 pub(crate) struct LtoBitcodeFromRlib {
     pub gcc_err: String,
 }
+
+pub(crate) struct TargetFeatureDisableOrEnable<'a> {
+    pub features: &'a [&'a str],
+    pub span: Option<Span>,
+    pub missing_features: Option<MissingFeatures>,
+}
+
+#[derive(Subdiagnostic)]
+#[help(codegen_gcc_missing_features)]
+pub(crate) struct MissingFeatures;
+
+impl IntoDiagnostic<'_, ErrorGuaranteed> for TargetFeatureDisableOrEnable<'_> {
+    fn into_diagnostic(self, sess: &'_ Handler) -> DiagnosticBuilder<'_, ErrorGuaranteed> {
+        let mut diag = sess.struct_err(fluent::codegen_gcc_target_feature_disable_or_enable);
+        if let Some(span) = self.span {
+            diag.set_span(span);
+        };
+        if let Some(missing_features) = self.missing_features {
+            diag.subdiagnostic(missing_features);
+        }
+        diag.set_arg("features", self.features.join(", "));
+        diag
+    }
+}
diff --git a/src/gcc_util.rs b/src/gcc_util.rs
new file mode 100644
index 00000000000..da1c0dfe559
--- /dev/null
+++ b/src/gcc_util.rs
@@ -0,0 +1,193 @@
+use smallvec::{smallvec, SmallVec};
+
+use rustc_codegen_ssa::target_features::{
+    supported_target_features, tied_target_features, RUSTC_SPECIFIC_FEATURES,
+};
+use rustc_data_structures::fx::FxHashMap;
+use rustc_middle::bug;
+use rustc_session::Session;
+
+use crate::errors::{PossibleFeature, TargetFeatureDisableOrEnable, UnknownCTargetFeature, UnknownCTargetFeaturePrefix};
+
+/// The list of GCC features computed from CLI flags (`-Ctarget-cpu`, `-Ctarget-feature`,
+/// `--target` and similar).
+pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec<String> {
+    // Features that come earlier are overridden by conflicting features later in the string.
+    // Typically we'll want more explicit settings to override the implicit ones, so:
+    //
+    // * Features from -Ctarget-cpu=*; are overridden by [^1]
+    // * Features implied by --target; are overridden by
+    // * Features from -Ctarget-feature; are overridden by
+    // * function specific features.
+    //
+    // [^1]: target-cpu=native is handled here, other target-cpu values are handled implicitly
+    // through GCC TargetMachine implementation.
+    //
+    // FIXME(nagisa): it isn't clear what's the best interaction between features implied by
+    // `-Ctarget-cpu` and `--target` are. On one hand, you'd expect CLI arguments to always
+    // override anything that's implicit, so e.g. when there's no `--target` flag, features implied
+    // the host target are overridden by `-Ctarget-cpu=*`. On the other hand, what about when both
+    // `--target` and `-Ctarget-cpu=*` are specified? Both then imply some target features and both
+    // flags are specified by the user on the CLI. It isn't as clear-cut which order of precedence
+    // should be taken in cases like these.
+    let mut features = vec![];
+
+    // TODO(antoyo): -Ctarget-cpu=native
+
+    // Features implied by an implicit or explicit `--target`.
+    features.extend(
+        sess.target
+            .features
+            .split(',')
+            .filter(|v| !v.is_empty() && backend_feature_name(v).is_some())
+            .map(String::from),
+    );
+
+    // -Ctarget-features
+    let supported_features = supported_target_features(sess);
+    let mut featsmap = FxHashMap::default();
+    let feats = sess.opts.cg.target_feature
+        .split(',')
+        .filter_map(|s| {
+            let enable_disable = match s.chars().next() {
+                None => return None,
+                Some(c @ ('+' | '-')) => c,
+                Some(_) => {
+                    if diagnostics {
+                        sess.emit_warning(UnknownCTargetFeaturePrefix { feature: s });
+                    }
+                    return None;
+                }
+            };
+
+            let feature = backend_feature_name(s)?;
+            // Warn against use of GCC specific feature names on the CLI.
+            if diagnostics && !supported_features.iter().any(|&(v, _)| v == feature) {
+                let rust_feature = supported_features.iter().find_map(|&(rust_feature, _)| {
+                    let gcc_features = to_gcc_features(sess, rust_feature);
+                    if gcc_features.contains(&feature) && !gcc_features.contains(&rust_feature) {
+                        Some(rust_feature)
+                    } else {
+                        None
+                    }
+                });
+                let unknown_feature =
+                    if let Some(rust_feature) = rust_feature {
+                        UnknownCTargetFeature {
+                            feature,
+                            rust_feature: PossibleFeature::Some { rust_feature },
+                        }
+                    }
+                    else {
+                        UnknownCTargetFeature { feature, rust_feature: PossibleFeature::None }
+                    };
+                sess.emit_warning(unknown_feature);
+            }
+
+            if diagnostics {
+                // FIXME(nagisa): figure out how to not allocate a full hashset here.
+                featsmap.insert(feature, enable_disable == '+');
+            }
+
+            // rustc-specific features do not get passed down to GCC…
+            if RUSTC_SPECIFIC_FEATURES.contains(&feature) {
+                return None;
+            }
+            // ... otherwise though we run through `to_gcc_features` when
+            // passing requests down to GCC. This means that all in-language
+            // features also work on the command line instead of having two
+            // different names when the GCC name and the Rust name differ.
+            Some(to_gcc_features(sess, feature)
+                .iter()
+                .flat_map(|feat| to_gcc_features(sess, feat).into_iter())
+                .map(String::from)
+                .collect::<Vec<_>>(),
+            )
+        })
+        .flatten();
+    features.extend(feats);
+
+    if diagnostics {
+        if let Some(f) = check_tied_features(sess, &featsmap) {
+            sess.emit_err(TargetFeatureDisableOrEnable {
+                features: f,
+                span: None,
+                missing_features: None,
+            });
+        }
+    }
+
+    features
+}
+
+/// Returns a feature name for the given `+feature` or `-feature` string.
+///
+/// Only allows features that are backend specific (i.e. not [`RUSTC_SPECIFIC_FEATURES`].)
+fn backend_feature_name(s: &str) -> Option<&str> {
+    // features must start with a `+` or `-`.
+    let feature = s.strip_prefix(&['+', '-'][..]).unwrap_or_else(|| {
+        bug!("target feature `{}` must begin with a `+` or `-`", s);
+    });
+    // Rustc-specific feature requests like `+crt-static` or `-crt-static`
+    // are not passed down to GCC.
+    if RUSTC_SPECIFIC_FEATURES.contains(&feature) {
+        return None;
+    }
+    Some(feature)
+}
+
+// TODO(antoyo): maybe move to a new module gcc_util.
+// To find a list of GCC's names, check https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
+pub fn to_gcc_features<'a>(sess: &Session, s: &'a str) -> SmallVec<[&'a str; 2]> {
+    let arch = if sess.target.arch == "x86_64" { "x86" } else { &*sess.target.arch };
+    match (arch, s) {
+        ("x86", "sse4.2") => smallvec!["sse4.2", "crc32"],
+        ("x86", "pclmulqdq") => smallvec!["pclmul"],
+        ("x86", "rdrand") => smallvec!["rdrnd"],
+        ("x86", "bmi1") => smallvec!["bmi"],
+        ("x86", "cmpxchg16b") => smallvec!["cx16"],
+        ("x86", "avx512vaes") => smallvec!["vaes"],
+        ("x86", "avx512gfni") => smallvec!["gfni"],
+        ("x86", "avx512vpclmulqdq") => smallvec!["vpclmulqdq"],
+        // NOTE: seems like GCC requires 'avx512bw' for 'avx512vbmi2'.
+        ("x86", "avx512vbmi2") => smallvec!["avx512vbmi2", "avx512bw"],
+        // NOTE: seems like GCC requires 'avx512bw' for 'avx512bitalg'.
+        ("x86", "avx512bitalg") => smallvec!["avx512bitalg", "avx512bw"],
+        ("aarch64", "rcpc2") => smallvec!["rcpc-immo"],
+        ("aarch64", "dpb") => smallvec!["ccpp"],
+        ("aarch64", "dpb2") => smallvec!["ccdp"],
+        ("aarch64", "frintts") => smallvec!["fptoint"],
+        ("aarch64", "fcma") => smallvec!["complxnum"],
+        ("aarch64", "pmuv3") => smallvec!["perfmon"],
+        ("aarch64", "paca") => smallvec!["pauth"],
+        ("aarch64", "pacg") => smallvec!["pauth"],
+        // Rust ties fp and neon together. In GCC neon implicitly enables fp,
+        // but we manually enable neon when a feature only implicitly enables fp
+        ("aarch64", "f32mm") => smallvec!["f32mm", "neon"],
+        ("aarch64", "f64mm") => smallvec!["f64mm", "neon"],
+        ("aarch64", "fhm") => smallvec!["fp16fml", "neon"],
+        ("aarch64", "fp16") => smallvec!["fullfp16", "neon"],
+        ("aarch64", "jsconv") => smallvec!["jsconv", "neon"],
+        ("aarch64", "sve") => smallvec!["sve", "neon"],
+        ("aarch64", "sve2") => smallvec!["sve2", "neon"],
+        ("aarch64", "sve2-aes") => smallvec!["sve2-aes", "neon"],
+        ("aarch64", "sve2-sm4") => smallvec!["sve2-sm4", "neon"],
+        ("aarch64", "sve2-sha3") => smallvec!["sve2-sha3", "neon"],
+        ("aarch64", "sve2-bitperm") => smallvec!["sve2-bitperm", "neon"],
+        (_, s) => smallvec![s],
+    }
+}
+
+// Given a map from target_features to whether they are enabled or disabled,
+// ensure only valid combinations are allowed.
+pub fn check_tied_features(sess: &Session, features: &FxHashMap<&str, bool>) -> Option<&'static [&'static str]> {
+    for tied in tied_target_features(sess) {
+        // Tied features must be set to the same value, or not set at all
+        let mut tied_iter = tied.iter();
+        let enabled = features.get(tied_iter.next().unwrap());
+        if tied_iter.any(|feature| enabled != features.get(feature)) {
+            return Some(tied);
+        }
+    }
+    None
+}
diff --git a/src/lib.rs b/src/lib.rs
index 9dbe6aab8cb..eedac315c60 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -65,6 +65,7 @@ mod coverageinfo;
 mod debuginfo;
 mod declare;
 mod errors;
+mod gcc_util;
 mod int;
 mod intrinsic;
 mod mono_item;
@@ -195,8 +196,8 @@ impl CodegenBackend for GccCodegenBackend {
     }
 
     fn provide(&self, providers: &mut Providers) {
-        // FIXME(antoyo) compute list of enabled features from cli flags
-        providers.global_backend_features = |_tcx, ()| vec![];
+        providers.global_backend_features =
+            |tcx, ()| gcc_util::global_gcc_features(tcx.sess, true)
     }
 
     fn codegen_crate<'tcx>(&self, tcx: TyCtxt<'tcx>, metadata: EncodedMetadata, need_metadata_module: bool) -> Box<dyn Any> {