Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use preserve_mostcc for extern "rust-cold" #115260

Merged
merged 1 commit into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ fn clif_sig_from_fn_abi<'tcx>(
pub(crate) fn conv_to_call_conv(sess: &Session, c: Conv, default_call_conv: CallConv) -> CallConv {
match c {
Conv::Rust | Conv::C => default_call_conv,
Conv::RustCold => CallConv::Cold,
Conv::Cold | Conv::PreserveMost | Conv::PreserveAll => CallConv::Cold,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these are ABI compatible between LLVM and Cranelift actually. So using them in the standard library will break ABI compatibility. It should be possible to implement in Cranelift if LLVM guarantees a specific call conv for them, but that doesn't help for GCC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess that's not a blocker for this PR, though, since it was coldcc before, which if I understand correctly already wasn't ABI compatible.

What's your meta expectation here for the future? Do all externs have to be interoperable between backends? That would seem a shame, as it would essentially force extern "Rust" to always be the same as extern "C", rather than letting a backend innovate something faster. Would it be ok to have options to force rust-cold to be something else (maybe just C) for people who need the interop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all externs have to be interoperable between backends?

Preferably, but if any is not interoperable, denying usage in the standard library and emitting a hard error if caller and callee are compiled with a different codegen backend would work too I guess. (eg by internally lowering it to a different call conv depending on the selected codegen backend before we do typeck)

That would seem a shame, as it would essentially force extern "Rust" to always be the same as extern "C", rather than letting a backend innovate something faster.

extern "Rust" could be changed to internally use eg extern "sysv" on all targets or extern "win64" on all x86_64 targets. Note that extern "Rust" is already not extern "C". The lowering from rust types to primitives is different, but those primitives are passed in the same way such that it is actually possible for all backends to use the same ABI by reusing the lowering code in rustc_target. I don't want to change that.

Would it be ok to have options to force rust-cold to be something else (maybe just C) for people who need the interop?

That wouldn't help if the standard library uses it as the LLVM compiled standard library will be used with cg_clif once it ships on nightly.

Conv::X86_64SysV => CallConv::SystemV,
Conv::X86_64Win64 => CallConv::WindowsFastcall,

Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_llvm/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,9 @@ impl From<Conv> for llvm::CallConv {
Conv::C | Conv::Rust | Conv::CCmseNonSecureCall | Conv::RiscvInterrupt { .. } => {
llvm::CCallConv
}
Conv::RustCold => llvm::ColdCallConv,
Conv::Cold => llvm::ColdCallConv,
Conv::PreserveMost => llvm::PreserveMost,
Conv::PreserveAll => llvm::PreserveAll,
Conv::AmdGpuKernel => llvm::AmdGpuKernel,
Conv::AvrInterrupt => llvm::AvrInterrupt,
Conv::AvrNonBlockingInterrupt => llvm::AvrNonBlockingInterrupt,
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,17 @@ pub enum LLVMModFlagBehavior {
// Consts for the LLVM CallConv type, pre-cast to usize.

/// LLVM CallingConv::ID. Should we wrap this?
///
/// See <https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/CallingConv.h>
#[derive(Copy, Clone, PartialEq, Debug)]
#[repr(C)]
pub enum CallConv {
CCallConv = 0,
FastCallConv = 8,
ColdCallConv = 9,
PreserveMost = 14,
PreserveAll = 15,
Tail = 18,
X86StdcallCallConv = 64,
X86FastcallCallConv = 65,
ArmAapcsCallConv = 67,
Expand Down
11 changes: 4 additions & 7 deletions compiler/rustc_target/src/abi/call/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -579,10 +579,9 @@ pub enum Conv {
C,
Rust,

/// For things unlikely to be called, where smaller caller codegen is
/// preferred over raw speed.
/// Stronger than just `#[cold]` because `fn` pointers might be incompatible.
RustCold,
Cold,
PreserveMost,
PreserveAll,

// Target-specific calling conventions.
ArmAapcs,
Expand All @@ -605,9 +604,7 @@ pub enum Conv {
AvrInterrupt,
AvrNonBlockingInterrupt,

RiscvInterrupt {
kind: RiscvInterruptKind,
},
RiscvInterrupt { kind: RiscvInterruptKind },
}

#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)]
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_target/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ impl ToJson for crate::abi::call::Conv {
let s = match self {
Self::C => "C",
Self::Rust => "Rust",
Self::RustCold => "RustCold",
Self::Cold => "Cold",
Self::PreserveMost => "PreserveMost",
Self::PreserveAll => "PreserveAll",
Self::ArmAapcs => "ArmAapcs",
Self::CCmseNonSecureCall => "CCmseNonSecureCall",
Self::Msp430Intr => "Msp430Intr",
Expand Down
43 changes: 33 additions & 10 deletions compiler/rustc_target/src/spec/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,33 @@ pub enum Abi {
// hashing tests. These are used in many places, so giving them stable values reduces test
// churn. The specific values are meaningless.
Rust,
C { unwind: bool },
Cdecl { unwind: bool },
Stdcall { unwind: bool },
Fastcall { unwind: bool },
Vectorcall { unwind: bool },
Thiscall { unwind: bool },
Aapcs { unwind: bool },
Win64 { unwind: bool },
SysV64 { unwind: bool },
C {
unwind: bool,
},
Cdecl {
unwind: bool,
},
Stdcall {
unwind: bool,
},
Fastcall {
unwind: bool,
},
Vectorcall {
unwind: bool,
},
Thiscall {
unwind: bool,
},
Aapcs {
unwind: bool,
},
Win64 {
unwind: bool,
},
SysV64 {
unwind: bool,
},
PtxKernel,
Msp430Interrupt,
X86Interrupt,
Expand All @@ -32,11 +50,16 @@ pub enum Abi {
AvrNonBlockingInterrupt,
CCmseNonSecureCall,
Wasm,
System { unwind: bool },
System {
unwind: bool,
},
RustIntrinsic,
RustCall,
PlatformIntrinsic,
Unadjusted,
/// For things unlikely to be called, where reducing register pressure in
/// `extern "Rust"` callers is worth paying extra cost in the callee.
/// Stronger than just `#[cold]` because `fn` pointers might be incompatible.
RustCold,
RiscvInterruptM,
RiscvInterruptS,
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,13 @@ impl Target {
Abi::Vectorcall { .. } if ["x86", "x86_64"].contains(&&self.arch[..]) => abi,
Abi::Fastcall { unwind } | Abi::Vectorcall { unwind } => Abi::C { unwind },

// The Windows x64 calling convention we use for `extern "Rust"`
// <https://learn.microsoft.com/en-us/cpp/build/x64-software-conventions#register-volatility-and-preservation>
// expects the callee to save `xmm6` through `xmm15`, but `PreserveMost`
// (that we use by default for `extern "rust-cold"`) doesn't save any of those.
// So to avoid bloating callers, just use the Rust convention here.
Abi::RustCold if self.is_like_windows && self.arch == "x86_64" => Abi::Rust,

abi => abi,
}
}
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_ty_utils/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,10 @@ fn conv_from_spec_abi(tcx: TyCtxt<'_>, abi: SpecAbi) -> Conv {
use rustc_target::spec::abi::Abi::*;
match tcx.sess.target.adjust_abi(abi) {
RustIntrinsic | PlatformIntrinsic | Rust | RustCall => Conv::Rust,
RustCold => Conv::RustCold,

// This is intentionally not using `Conv::Cold`, as that has to preserve
// even SIMD registers, which is generally not a good trade-off.
RustCold => Conv::PreserveMost,

// It's the ABI's job to select this, not ours.
System { .. } => bug!("system abi should be selected elsewhere"),
Expand Down
13 changes: 11 additions & 2 deletions tests/codegen/cold-call-declare-and-call.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
// revisions: NORMAL WINDOWS
// compile-flags: -C no-prepopulate-passes
//[NORMAL] ignore-windows
//[WINDOWS] only-windows
//[WINDOWS] only-x86_64

#![crate_type = "lib"]
#![feature(rust_cold_cc)]

// wasm marks the definition as `dso_local`, so allow that as optional.

// CHECK: define{{( dso_local)?}} coldcc void @this_should_never_happen(i16
// CHECK: call coldcc void @this_should_never_happen(i16
// NORMAL: define{{( dso_local)?}} preserve_mostcc void @this_should_never_happen(i16
// NORMAL: call preserve_mostcc void @this_should_never_happen(i16

// See the comment in `Target::adjust_abi` for why this differs

// WINDOWS: define void @this_should_never_happen(i16
// WINDOWS: call void @this_should_never_happen(i16

#[no_mangle]
pub extern "rust-cold" fn this_should_never_happen(x: u16) {}
Expand Down