Refactor emitting errors to declare.rs, and use fluently-generated error messages

ss

Author: sayantn

compiler/rustc_codegen_llvm/messages.ftl

@@ -3,6 +3,12 @@ codegen_llvm_autodiff_without_lto = using the autodiff feature requires using fa
 
 codegen_llvm_copy_bitcode = failed to copy bitcode to object file: {$err}
 
+codegen_llvm_deprecated_intrinsic =
+    Using deprecated intrinsic `{$name}`, consider using other intrinsics/instructions
+
+codegen_llvm_deprecated_intrinsic_with_replacement =
+    Using deprecated intrinsic `{$name}`, `{$replacement}` can be used instead
+
 codegen_llvm_dynamic_linking_with_lto =
     cannot prefer dynamic linking when performing LTO
     .note = only 'staticlib', 'bin', and 'cdylib' outputs are supported with LTO
@@ -19,6 +25,12 @@ codegen_llvm_from_llvm_diag = {$message}
 
 codegen_llvm_from_llvm_optimization_diag = {$filename}:{$line}:{$column} {$pass_name} ({$kind}): {$message}
 
+codegen_llvm_intrinsic_signature_mismatch =
+    Intrinsic signature mismatch for `{$name}`: expected signature `{$llvm_fn_ty}`, found `{$rust_fn_ty}`
+
+codegen_llvm_invalid_intrinsic =
+    Invalid LLVM Intrinsic `{$name}`
+
 codegen_llvm_load_bitcode = failed to load bitcode of module "{$name}"
 codegen_llvm_load_bitcode_with_llvm_err = failed to load bitcode of module "{$name}": {$llvm_err}
 

compiler/rustc_codegen_llvm/src/abi.rs

@@ -302,20 +302,32 @@ impl<'ll, 'tcx> ArgAbiBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
 }
 
 pub(crate) enum FunctionSignature<'ll> {
-    /// The signature is obtained directly from LLVM, and **may not match the Rust signature**
-    Intrinsic(&'ll Type),
+    /// This is an LLVM intrinsic, the signature is obtained directly from LLVM, and **may not match the Rust signature**
+    LLVMSignature(llvm::Intrinsic, &'ll Type),
+    /// This is an LLVM intrinsic, but the signature is just the Rust signature.
+    /// FIXME: this should ideally not exist, we should be using the LLVM signature for all LLVM intrinsics
+    RustSignature(llvm::Intrinsic, &'ll Type),
     /// The name starts with `llvm.`, but can't obtain the intrinsic ID. May be invalid or upgradable
-    MaybeInvalidIntrinsic(&'ll Type),
+    MaybeInvalid(&'ll Type),
     /// Just the Rust signature
-    Rust(&'ll Type),
+    NotIntrinsic(&'ll Type),
 }
 
 impl<'ll> FunctionSignature<'ll> {
     pub(crate) fn fn_ty(&self) -> &'ll Type {
         match self {
-            FunctionSignature::Intrinsic(fn_ty)
-            | FunctionSignature::MaybeInvalidIntrinsic(fn_ty)
-            | FunctionSignature::Rust(fn_ty) => fn_ty,
+            FunctionSignature::LLVMSignature(_, fn_ty)
+            | FunctionSignature::RustSignature(_, fn_ty)
+            | FunctionSignature::MaybeInvalid(fn_ty)
+            | FunctionSignature::NotIntrinsic(fn_ty) => fn_ty,
+        }
+    }
+
+    pub(crate) fn intrinsic(&self) -> Option<llvm::Intrinsic> {
+        match self {
+            FunctionSignature::RustSignature(intrinsic, _)
+            | FunctionSignature::LLVMSignature(intrinsic, _) => Some(*intrinsic),
+            _ => None,
         }
     }
 }
@@ -326,12 +338,9 @@ pub(crate) trait FnAbiLlvmExt<'ll, 'tcx> {
     /// When `do_verify` is set, this function performs checks for the signature of LLVM intrinsics
     /// and emits a fatal error if it doesn't match. These checks are important,but somewhat expensive
     /// So they are only used at function definitions, not at callsites
-    fn llvm_type(
-        &self,
-        cx: &CodegenCx<'ll, 'tcx>,
-        name: &[u8],
-        do_verify: bool,
-    ) -> FunctionSignature<'ll>;
+    fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>, name: &[u8]) -> FunctionSignature<'ll>;
+    /// The normal Rust signature for this
+    fn rust_signature(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type;
     /// **If this function is an LLVM intrinsic** checks if the LLVM signature provided matches with this
     fn verify_intrinsic_signature(&self, cx: &CodegenCx<'ll, 'tcx>, llvm_ty: &'ll Type) -> bool;
     fn ptr_to_llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type;
@@ -499,58 +508,41 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
             return false;
         }
 
-        // todo: add bypasses for types not accessible from Rust here
         iter::once((rust_return_ty, llvm_return_ty))
             .chain(iter::zip(rust_argument_tys, llvm_argument_tys))
             .all(|(rust_ty, llvm_ty)| cx.equate_ty(rust_ty, llvm_ty))
     }
 
-    fn llvm_type(
-        &self,
-        cx: &CodegenCx<'ll, 'tcx>,
-        name: &[u8],
-        do_verify: bool,
-    ) -> FunctionSignature<'ll> {
-        let mut maybe_invalid = false;
+    fn rust_signature(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type {
+        let return_ty = self.llvm_return_type(cx);
+        let argument_tys = self.llvm_argument_types(cx);
+
+        if self.c_variadic {
+            cx.type_variadic_func(&argument_tys, return_ty)
+        } else {
+            cx.type_func(&argument_tys, return_ty)
+        }
+    }
 
+    fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>, name: &[u8]) -> FunctionSignature<'ll> {
         if name.starts_with(b"llvm.") {
             if let Some(intrinsic) = llvm::Intrinsic::lookup(name) {
                 if !intrinsic.is_overloaded() {
                     // FIXME: also do this for overloaded intrinsics
-                    let llvm_fn_ty = cx.intrinsic_type(intrinsic, &[]);
-                    if do_verify {
-                        if !self.verify_intrinsic_signature(cx, llvm_fn_ty) {
-                            cx.tcx.dcx().fatal(format!(
-                                "Intrinsic signature mismatch for `{}`: expected signature `{llvm_fn_ty:?}`",
-                                str::from_utf8(name).unwrap()
-                            ));
-                        }
-                    }
-                    return FunctionSignature::Intrinsic(llvm_fn_ty);
+                    FunctionSignature::LLVMSignature(intrinsic, cx.intrinsic_type(intrinsic, &[]))
+                } else {
+                    FunctionSignature::RustSignature(intrinsic, self.rust_signature(cx))
                 }
             } else {
                 // it's one of 2 cases,
                 // - either the base name is invalid
                 // - it has been superceded by something else, so the intrinsic was removed entirely
                 // to check for upgrades, we need the `llfn`, so we defer it for now
 
-                maybe_invalid = true;
+                FunctionSignature::MaybeInvalid(self.rust_signature(cx))
             }
-        }
-
-        let return_ty = self.llvm_return_type(cx);
-        let argument_tys = self.llvm_argument_types(cx);
-
-        let fn_ty = if self.c_variadic {
-            cx.type_variadic_func(&argument_tys, return_ty)
         } else {
-            cx.type_func(&argument_tys, return_ty)
-        };
-
-        if maybe_invalid {
-            FunctionSignature::MaybeInvalidIntrinsic(fn_ty)
-        } else {
-            FunctionSignature::Rust(fn_ty)
+            FunctionSignature::NotIntrinsic(self.rust_signature(cx))
         }
     }
 
@@ -699,15 +691,9 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
         callsite: &'ll Value,
         llfn: &'ll Value,
     ) {
-        // if we are using the LLVM signature, use the LLVM attributes otherwise it might be problematic
-        let name = llvm::get_value_name(llfn);
-        if name.starts_with(b"llvm.")
-            && let Some(intrinsic) = llvm::Intrinsic::lookup(name)
-        {
-            // FIXME: also do this for overloaded intrinsics
-            if !intrinsic.is_overloaded() {
-                return;
-            }
+        // Don't apply any attributes to LLVM intrinsics, they will be applied by AutoUpgrade
+        if llvm::get_value_name(llfn).starts_with(b"llvm.") {
+            return;
         }
 
         let mut func_attrs = SmallVec::<[_; 2]>::new();

compiler/rustc_codegen_llvm/src/declare.rs

@@ -29,7 +29,7 @@ use crate::llvm::AttributePlace::Function;
 use crate::llvm::Visibility;
 use crate::type_::Type;
 use crate::value::Value;
-use crate::{attributes, llvm};
+use crate::{attributes, errors, llvm};
 
 /// Declare a function with a SimpleCx.
 ///
@@ -150,52 +150,59 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     ) -> &'ll Value {
         debug!("declare_rust_fn(name={:?}, fn_abi={:?})", name, fn_abi);
 
-        let signature = fn_abi.llvm_type(self, name.as_bytes(), true);
-        let llfn;
+        let signature = fn_abi.llvm_type(self, name.as_bytes());
 
-        if let FunctionSignature::Intrinsic(fn_ty) = signature {
-            // intrinsics have a specified set of attributes, so we don't use the `FnAbi` set for them
-            llfn = declare_simple_fn(
-                self,
-                name,
-                fn_abi.llvm_cconv(self),
-                llvm::UnnamedAddr::Global,
-                llvm::Visibility::Default,
-                fn_ty,
-            );
-        } else {
-            // Function addresses in Rust are never significant, allowing functions to
-            // be merged.
-            llfn = declare_raw_fn(
-                self,
-                name,
-                fn_abi.llvm_cconv(self),
-                llvm::UnnamedAddr::Global,
-                llvm::Visibility::Default,
-                signature.fn_ty(),
-            );
+        let span = || instance.map(|instance| self.tcx.def_span(instance.def_id()));
+
+        if let FunctionSignature::LLVMSignature(_, llvm_fn_ty) = signature {
+            // check if the intrinsic signatures match
+            if !fn_abi.verify_intrinsic_signature(self, llvm_fn_ty) {
+                self.tcx.dcx().emit_fatal(errors::IntrinsicSignatureMismatch {
+                    name,
+                    llvm_fn_ty: &format!("{llvm_fn_ty:?}"),
+                    rust_fn_ty: &format!("{:?}", fn_abi.rust_signature(self)),
+                    span: span(),
+                });
+            }
+        }
+
+        // Function addresses in Rust are never significant, allowing functions to
+        // be merged.
+        let llfn = declare_raw_fn(
+            self,
+            name,
+            fn_abi.llvm_cconv(self),
+            llvm::UnnamedAddr::Global,
+            llvm::Visibility::Default,
+            signature.fn_ty(),
+        );
+
+        if signature.intrinsic().is_none() {
+            // Don't apply any attributes to intrinsics, they will be applied by AutoUpgrade
             fn_abi.apply_attrs_llfn(self, llfn, instance);
         }
 
-        if let FunctionSignature::MaybeInvalidIntrinsic(..) = signature {
+        if let FunctionSignature::MaybeInvalid(..) = signature {
             let mut new_llfn = None;
             let can_upgrade =
                 unsafe { llvm::LLVMRustUpgradeIntrinsicFunction(llfn, &mut new_llfn, false) };
 
             if can_upgrade {
                 // not all intrinsics are upgraded to some other intrinsics, most are upgraded to instruction sequences
                 if let Some(new_llfn) = new_llfn {
-                    self.tcx.dcx().note(format!(
-                        "Using deprecated intrinsic `{name}`, `{}` can be used instead",
-                        str::from_utf8(llvm::get_value_name(new_llfn)).unwrap()
-                    ));
+                    self.tcx.dcx().emit_note(errors::DeprecatedIntrinsicWithReplacement {
+                        name,
+                        replacement: str::from_utf8(llvm::get_value_name(new_llfn)).unwrap(),
+                        span: span(),
+                    });
                 } else if self.tcx.sess.opts.verbose {
-                    self.tcx.dcx().note(format!(
-                        "Using deprecated intrinsic `{name}`, consider using other intrinsics/instructions"
-                    ));
+                    // At least for now, we are only emitting notes for deprecated intrinsics with no direct replacement
+                    // because they are used quite a lot in stdarch. After the stdarch uses has been removed, we can make
+                    // this always emit a note (or even an warning)
+                    self.tcx.dcx().emit_note(errors::DeprecatedIntrinsic { name, span: span() });
                 }
             } else {
-                self.tcx.dcx().fatal(format!("Invalid LLVM intrinsic: `{name}`"))
+                self.tcx.dcx().emit_fatal(errors::InvalidIntrinsic { name, span: span() });
             }
         }
 

compiler/rustc_codegen_llvm/src/errors.rs

@@ -209,3 +209,38 @@ pub(crate) struct FixedX18InvalidArch<'a> {
 #[derive(Diagnostic)]
 #[diag(codegen_llvm_sanitizer_kcfi_arity_requires_llvm_21_0_0)]
 pub(crate) struct SanitizerKcfiArityRequiresLLVM2100;
+
+#[derive(Diagnostic)]
+#[diag(codegen_llvm_intrinsic_signature_mismatch)]
+pub(crate) struct IntrinsicSignatureMismatch<'a> {
+    pub name: &'a str,
+    pub llvm_fn_ty: &'a str,
+    pub rust_fn_ty: &'a str,
+    #[primary_span]
+    pub span: Option<Span>,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_llvm_invalid_intrinsic)]
+pub(crate) struct InvalidIntrinsic<'a> {
+    pub name: &'a str,
+    #[primary_span]
+    pub span: Option<Span>,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_llvm_deprecated_intrinsic)]
+pub(crate) struct DeprecatedIntrinsic<'a> {
+    pub name: &'a str,
+    #[primary_span]
+    pub span: Option<Span>,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_llvm_deprecated_intrinsic_with_replacement)]
+pub(crate) struct DeprecatedIntrinsicWithReplacement<'a> {
+    pub name: &'a str,
+    pub replacement: &'a str,
+    #[primary_span]
+    pub span: Option<Span>,
+}

compiler/rustc_codegen_llvm/src/intrinsic.rs

@@ -1067,7 +1067,7 @@ fn gen_fn<'a, 'll, 'tcx>(
     codegen: &mut dyn FnMut(Builder<'a, 'll, 'tcx>),
 ) -> (&'ll Type, &'ll Value) {
     let fn_abi = cx.fn_abi_of_fn_ptr(rust_fn_sig, ty::List::empty());
-    let llty = fn_abi.llvm_type(cx, name.as_bytes(), true).fn_ty();
+    let llty = fn_abi.llvm_type(cx, name.as_bytes()).fn_ty();
     let llfn = cx.declare_fn(name, fn_abi, None);
     cx.set_frame_pointer_type(llfn);
     cx.apply_target_cpu_attr(llfn);

compiler/rustc_codegen_llvm/src/type_.rs

@@ -307,7 +307,7 @@ impl<'ll, 'tcx> LayoutTypeCodegenMethods<'tcx> for CodegenCx<'ll, 'tcx> {
         fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
         fn_ptr: &'ll Value,
     ) -> &'ll Type {
-        fn_abi.llvm_type(self, llvm::get_value_name(fn_ptr), false).fn_ty()
+        fn_abi.llvm_type(self, llvm::get_value_name(fn_ptr)).fn_ty()
     }
     fn fn_ptr_backend_type(&self, fn_abi: &FnAbi<'tcx, Ty<'tcx>>) -> &'ll Type {
         fn_abi.ptr_to_llvm_type(self)

tests/ui/codegen/deprecated-llvm-intrinsic.rs

@@ -18,11 +18,10 @@ pub struct i8x8([i8; 8]);
 extern "unadjusted" {
     #[link_name = "llvm.aarch64.neon.rbit.v8i8"]
     fn foo(a: i8x8) -> i8x8;
+    //~^ NOTE: Using deprecated intrinsic `llvm.aarch64.neon.rbit.v8i8`, `llvm.bitreverse.v8i8` can be used instead
 }
 
 #[target_feature(enable = "neon")]
 pub unsafe fn bar(a: i8x8) -> i8x8 {
     foo(a)
 }
-
-//~? NOTE: Using deprecated intrinsic `llvm.aarch64.neon.rbit.v8i8`, `llvm.bitreverse.v8i8` can be used instead

tests/ui/codegen/deprecated-llvm-intrinsic.stderr

@@ -1,2 +1,6 @@
 note: Using deprecated intrinsic `llvm.aarch64.neon.rbit.v8i8`, `llvm.bitreverse.v8i8` can be used instead
+  --> $DIR/deprecated-llvm-intrinsic.rs:20:5
+   |
+LL |     fn foo(a: i8x8) -> i8x8;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
 

tests/ui/codegen/incorrect-llvm-intrinsic-signature.rs

@@ -1,15 +1,13 @@
 //@ build-fail
 
 #![feature(link_llvm_intrinsics, abi_unadjusted)]
-#![allow(internal_features, non_camel_case_types, improper_ctypes)]
 
 extern "unadjusted" {
     #[link_name = "llvm.assume"]
     fn foo();
+    //~^ ERROR: Intrinsic signature mismatch for `llvm.assume`: expected signature `void (i1)`, found `void ()`
 }
 
 pub fn main() {
     unsafe { foo() }
 }
-
-//~? ERROR: Intrinsic signature mismatch for `llvm.assume`: expected signature `void (i1)`

tests/ui/codegen/incorrect-llvm-intrinsic-signature.stderr

@@ -1,4 +1,8 @@
-error: Intrinsic signature mismatch for `llvm.assume`: expected signature `void (i1)`
+error: Intrinsic signature mismatch for `llvm.assume`: expected signature `void (i1)`, found `void ()`
+  --> $DIR/incorrect-llvm-intrinsic-signature.rs:7:5
+   |
+LL |     fn foo();
+   |     ^^^^^^^^^
 
 error: aborting due to 1 previous error