From 3cd9311ecba81efe626adc87a669e9c8e2be1026 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 14 Dec 2024 00:11:03 +0000 Subject: [PATCH 1/5] Implement `Copy` for `i128` and `u128` in minicore Attempting to use these types currently causes an `error performing operation: fully_perform`. --- tests/auxiliary/minicore.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/auxiliary/minicore.rs b/tests/auxiliary/minicore.rs index c4317752920f0..2379d3adf1b4c 100644 --- a/tests/auxiliary/minicore.rs +++ b/tests/auxiliary/minicore.rs @@ -40,8 +40,11 @@ impl LegacyReceiver for &mut T {} pub trait Copy: Sized {} impl_marker_trait!( - Copy => [ bool, char, isize, usize, i8, i16, i32, i64, u8, u16, u32, u64, f32, f64 ] + Copy => [ + bool, char, isize, usize, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64 + ] ); + impl<'a, T: ?Sized> Copy for &'a T {} impl Copy for *const T {} impl Copy for *mut T {} From 847247bbf4cf6c9899db7a0301e903ebd21d77f7 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 14 Dec 2024 00:13:37 +0000 Subject: [PATCH 2/5] Introduce a test for the `i128` calling convention on Windows Currently we both pass and return `i128` indirectly on Windows for MSVC and MinGW, but this will be adjusted. Introduce a test verifying the current state. --- tests/codegen/i128-x86-callconv.rs | 79 ++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 tests/codegen/i128-x86-callconv.rs diff --git a/tests/codegen/i128-x86-callconv.rs b/tests/codegen/i128-x86-callconv.rs new file mode 100644 index 0000000000000..a2acf9a2a2fe1 --- /dev/null +++ b/tests/codegen/i128-x86-callconv.rs @@ -0,0 +1,79 @@ +//! Verify that Rust implements the expected calling convention for `i128`/`u128`. + +//@ revisions: MSVC MINGW +//@ add-core-stubs +//@ [MSVC] needs-llvm-components: x86 +//@ [MINGW] needs-llvm-components: x86 +//@ [MSVC] compile-flags: --target x86_64-pc-windows-msvc +//@ [MINGW] compile-flags: --target x86_64-pc-windows-gnu +//@ [MSVC] filecheck-flags: --check-prefix=WIN +//@ [MINGW] filecheck-flags: --check-prefix=WIN + +#![crate_type = "lib"] +#![no_std] +#![no_core] +#![feature(no_core, lang_items)] + +extern crate minicore; + +extern "C" { + fn extern_call(arg0: i128); + fn extern_ret() -> i128; +} + +#[no_mangle] +pub extern "C" fn pass(_arg0: u32, arg1: i128) { + // CHECK-LABEL: @pass( + // i128 is passed indirectly on Windows. It should load the pointer to the stack and pass + // a pointer to that allocation. + // WIN-SAME: %_arg0, ptr{{.*}} %arg1) + // WIN: [[PASS:%[_0-9]+]] = alloca [16 x i8], align 16 + // WIN: [[LOADED:%[_0-9]+]] = load i128, ptr %arg1 + // WIN: store i128 [[LOADED]], ptr [[PASS]] + // WIN: call void @extern_call + unsafe { extern_call(arg1) }; +} + +// Check that we produce the correct return ABI +#[no_mangle] +pub extern "C" fn ret(_arg0: u32, arg1: i128) -> i128 { + // CHECK-LABEL: @ret( + // i128 is returned on the stack on Windows. + // FIXME: this ABI does not agree with Clang or MinGW GCC + // WIN-SAME: ptr{{.*}} sret([16 x i8]){{.*}} [[RET:%_[0-9]+]], i32{{.*}} %_arg0, ptr{{.*}} %arg1) + // WIN: [[LOADED:%[0-9]+]] = load i128, ptr %arg1 + // WIN: store i128 [[LOADED]], ptr [[RET]] + // WIN: ret void + arg1 +} + +// Check that we consume the correct return ABI +#[no_mangle] +pub extern "C" fn forward(dst: *mut i128) { + // CHECK-LABEL: @forward + // WIN-SAME: ptr{{.*}} %dst) + // WIN: [[RETURNED:%[_0-9]+]] = alloca [16 x i8], align 16 + // WIN: call void @extern_ret({{.*}} [[RETURNED]]) + // WIN: [[TMP:%[_0-9]+]] = load i128, ptr [[RETURNED]] + // WIN: store i128 [[TMP]], ptr %dst + // WIN: ret void + unsafe { *dst = extern_ret() }; +} + +#[repr(C)] +struct RetAggregate { + a: i32, + b: i128, +} + +#[no_mangle] +pub extern "C" fn ret_aggregate(_arg0: u32, arg1: i128) -> RetAggregate { + // CHECK-LABEL: @ret_aggregate( + // Aggregates should also be returned indirectly + // WIN-SAME: ptr{{.*}}sret([32 x i8]){{.*}}[[RET:%[_0-9]+]], i32{{.*}}%_arg0, ptr{{.*}}%arg1) + // WIN: [[LOADED:%[_0-9]+]] = load i128, ptr %arg1 + // WIN: [[GEP:%[_0-9]+]] = getelementptr{{.*}}, ptr [[RET]] + // WIN: store i128 [[LOADED]], ptr [[GEP]] + // WIN: ret void + RetAggregate { a: 1, b: arg1 } +} From c4e33004a9ca68c8041b13ad1a378c8c66be0768 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 14 Dec 2024 00:32:39 +0000 Subject: [PATCH 3/5] Windows x86: Change `i128` to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: https://github.com/rust-lang/rust/issues/134288 --- .../rustc_target/src/callconv/x86_win64.rs | 20 ++++++++++++------- tests/codegen/i128-x86-callconv.rs | 17 +++++++--------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_target/src/callconv/x86_win64.rs b/compiler/rustc_target/src/callconv/x86_win64.rs index 83d94cb11bafd..59f911f7d5a5d 100644 --- a/compiler/rustc_target/src/callconv/x86_win64.rs +++ b/compiler/rustc_target/src/callconv/x86_win64.rs @@ -1,4 +1,4 @@ -use rustc_abi::{BackendRepr, Float, Primitive}; +use rustc_abi::{BackendRepr, Float, Integer, Primitive, RegKind, Size}; use crate::abi::call::{ArgAbi, FnAbi, Reg}; use crate::spec::HasTargetSpec; @@ -6,7 +6,7 @@ use crate::spec::HasTargetSpec; // Win64 ABI: https://docs.microsoft.com/en-us/cpp/build/parameter-passing pub(crate) fn compute_abi_info(cx: &impl HasTargetSpec, fn_abi: &mut FnAbi<'_, Ty>) { - let fixup = |a: &mut ArgAbi<'_, Ty>| { + let fixup = |a: &mut ArgAbi<'_, Ty>, is_ret: bool| { match a.layout.backend_repr { BackendRepr::Uninhabited | BackendRepr::Memory { sized: false } => {} BackendRepr::ScalarPair(..) | BackendRepr::Memory { sized: true } => { @@ -23,11 +23,16 @@ pub(crate) fn compute_abi_info(cx: &impl HasTargetSpec, fn_abi: &mut FnAbi<' // (probably what clang calls "illegal vectors"). } BackendRepr::Scalar(scalar) => { - // Match what LLVM does for `f128` so that `compiler-builtins` builtins match up - // with what LLVM expects. - if a.layout.size.bytes() > 8 + if is_ret && matches!(scalar.primitive(), Primitive::Int(Integer::I128, _)) { + // `i128` is returned in xmm0 by Clang and GCC, no + // FIXME(#134288): This may change for the `-msvc` targets in the future. + let reg = Reg { kind: RegKind::Vector, size: Size::from_bits(128) }; + a.cast_to(reg); + } else if a.layout.size.bytes() > 8 && !matches!(scalar.primitive(), Primitive::Float(Float::F128)) { + // Match what LLVM does for `f128` so that `compiler-builtins` builtins match up + // with what LLVM expects. a.make_indirect(); } else { a.extend_integer_width_to(32); @@ -37,8 +42,9 @@ pub(crate) fn compute_abi_info(cx: &impl HasTargetSpec, fn_abi: &mut FnAbi<' }; if !fn_abi.ret.is_ignore() { - fixup(&mut fn_abi.ret); + fixup(&mut fn_abi.ret, true); } + for arg in fn_abi.args.iter_mut() { if arg.is_ignore() { // x86_64-pc-windows-gnu doesn't ignore ZSTs. @@ -50,6 +56,6 @@ pub(crate) fn compute_abi_info(cx: &impl HasTargetSpec, fn_abi: &mut FnAbi<' } continue; } - fixup(arg); + fixup(arg, false); } } diff --git a/tests/codegen/i128-x86-callconv.rs b/tests/codegen/i128-x86-callconv.rs index a2acf9a2a2fe1..ee33239be3a7a 100644 --- a/tests/codegen/i128-x86-callconv.rs +++ b/tests/codegen/i128-x86-callconv.rs @@ -38,12 +38,11 @@ pub extern "C" fn pass(_arg0: u32, arg1: i128) { #[no_mangle] pub extern "C" fn ret(_arg0: u32, arg1: i128) -> i128 { // CHECK-LABEL: @ret( - // i128 is returned on the stack on Windows. - // FIXME: this ABI does not agree with Clang or MinGW GCC - // WIN-SAME: ptr{{.*}} sret([16 x i8]){{.*}} [[RET:%_[0-9]+]], i32{{.*}} %_arg0, ptr{{.*}} %arg1) - // WIN: [[LOADED:%[0-9]+]] = load i128, ptr %arg1 - // WIN: store i128 [[LOADED]], ptr [[RET]] - // WIN: ret void + // i128 is returned in xmm0 on Windows + // FIXME(#134288): This may change for the `-msvc` targets in the future. + // WIN-SAME: i32{{.*}} %_arg0, ptr{{.*}} %arg1) + // WIN: [[LOADED:%[_0-9]+]] = load <16 x i8>, ptr %arg1 + // WIN-NEXT: ret <16 x i8> [[LOADED]] arg1 } @@ -52,10 +51,8 @@ pub extern "C" fn ret(_arg0: u32, arg1: i128) -> i128 { pub extern "C" fn forward(dst: *mut i128) { // CHECK-LABEL: @forward // WIN-SAME: ptr{{.*}} %dst) - // WIN: [[RETURNED:%[_0-9]+]] = alloca [16 x i8], align 16 - // WIN: call void @extern_ret({{.*}} [[RETURNED]]) - // WIN: [[TMP:%[_0-9]+]] = load i128, ptr [[RETURNED]] - // WIN: store i128 [[TMP]], ptr %dst + // WIN: [[RETURNED:%[_0-9]+]] = tail call <16 x i8> @extern_ret() + // WIN: store <16 x i8> [[RETURNED]], ptr %dst // WIN: ret void unsafe { *dst = extern_ret() }; } From 0b635865708001c171a7429f9ceec46f1ffaab68 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 15 Dec 2024 08:03:27 +0000 Subject: [PATCH 4/5] Windows x86: Change `i128` to return via the vector ABI with Cranelift Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu but Rust returns the type on the stack. The LLVM backend was changed to use the same convention as the C compilers; change Cranelift to do the same. Link: https://github.com/rust-lang/rust/issues/134288 --- .../rustc_codegen_cranelift/src/abi/mod.rs | 17 +++++------ compiler/rustc_codegen_cranelift/src/cast.rs | 22 ++------------ .../src/codegen_i128.rs | 30 +++++-------------- 3 files changed, 19 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/abi/mod.rs b/compiler/rustc_codegen_cranelift/src/abi/mod.rs index 2466bfe60c7ab..907097efa27f4 100644 --- a/compiler/rustc_codegen_cranelift/src/abi/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/abi/mod.rs @@ -122,7 +122,7 @@ impl<'tcx> FunctionCx<'_, '_, 'tcx> { &mut self, name: &str, params: Vec, - returns: Vec, + mut returns: Vec, args: &[Value], ) -> Cow<'_, [Value]> { // Pass i128 arguments by-ref on Windows. @@ -146,15 +146,14 @@ impl<'tcx> FunctionCx<'_, '_, 'tcx> { (params, args.into()) }; - // Return i128 using a return area pointer on Windows and s390x. - let adjust_ret_param = - if self.tcx.sess.target.is_like_windows || self.tcx.sess.target.arch == "s390x" { - returns.len() == 1 && returns[0].value_type == types::I128 - } else { - false - }; + // Return i128 using the vector ABI on Windows + let ret_scalar_i128 = returns.len() == 1 && returns[0].value_type == types::I128; + if ret_scalar_i128 && self.tcx.sess.target.is_like_windows { + returns[0].value_type = types::I64X2; + } - if adjust_ret_param { + if ret_scalar_i128 && self.tcx.sess.target.arch == "s390x" { + // Return i128 using a return area pointer on s390x. let mut params = params; let mut args = args.to_vec(); diff --git a/compiler/rustc_codegen_cranelift/src/cast.rs b/compiler/rustc_codegen_cranelift/src/cast.rs index 0b5cb1547fc69..4463631c524be 100644 --- a/compiler/rustc_codegen_cranelift/src/cast.rs +++ b/compiler/rustc_codegen_cranelift/src/cast.rs @@ -96,25 +96,9 @@ pub(crate) fn clif_int_or_float_cast( }, ); - if fx.tcx.sess.target.is_like_windows { - let ret = fx.lib_call( - &name, - vec![AbiParam::new(from_ty)], - vec![AbiParam::new(types::I64X2)], - &[from], - )[0]; - // FIXME(bytecodealliance/wasmtime#6104) use bitcast instead of store to get from i64x2 to i128 - let ret_ptr = fx.create_stack_slot(16, 16); - ret_ptr.store(fx, ret, MemFlags::trusted()); - ret_ptr.load(fx, types::I128, MemFlags::trusted()) - } else { - fx.lib_call( - &name, - vec![AbiParam::new(from_ty)], - vec![AbiParam::new(types::I128)], - &[from], - )[0] - } + fx.lib_call(&name, vec![AbiParam::new(from_ty)], vec![AbiParam::new(types::I128)], &[ + from, + ])[0] } else if to_ty == types::I8 || to_ty == types::I16 { // FIXME implement fcvt_to_*int_sat.i8/i16 let val = if to_signed { diff --git a/compiler/rustc_codegen_cranelift/src/codegen_i128.rs b/compiler/rustc_codegen_cranelift/src/codegen_i128.rs index 734574338d049..3a94337a6ebbc 100644 --- a/compiler/rustc_codegen_cranelift/src/codegen_i128.rs +++ b/compiler/rustc_codegen_cranelift/src/codegen_i128.rs @@ -33,28 +33,14 @@ pub(crate) fn maybe_codegen<'tcx>( (BinOp::Rem, true) => "__modti3", _ => unreachable!(), }; - if fx.tcx.sess.target.is_like_windows { - let args = [lhs.load_scalar(fx), rhs.load_scalar(fx)]; - let ret = fx.lib_call( - name, - vec![AbiParam::new(types::I128), AbiParam::new(types::I128)], - vec![AbiParam::new(types::I64X2)], - &args, - )[0]; - // FIXME(bytecodealliance/wasmtime#6104) use bitcast instead of store to get from i64x2 to i128 - let ret_place = CPlace::new_stack_slot(fx, lhs.layout()); - ret_place.to_ptr().store(fx, ret, MemFlags::trusted()); - Some(ret_place.to_cvalue(fx)) - } else { - let args = [lhs.load_scalar(fx), rhs.load_scalar(fx)]; - let ret_val = fx.lib_call( - name, - vec![AbiParam::new(types::I128), AbiParam::new(types::I128)], - vec![AbiParam::new(types::I128)], - &args, - )[0]; - Some(CValue::by_val(ret_val, lhs.layout())) - } + let args = [lhs.load_scalar(fx), rhs.load_scalar(fx)]; + let ret_val = fx.lib_call( + name, + vec![AbiParam::new(types::I128), AbiParam::new(types::I128)], + vec![AbiParam::new(types::I128)], + &args, + )[0]; + Some(CValue::by_val(ret_val, lhs.layout())) } BinOp::Lt | BinOp::Le | BinOp::Eq | BinOp::Ge | BinOp::Gt | BinOp::Ne | BinOp::Cmp => None, BinOp::Shl | BinOp::ShlUnchecked | BinOp::Shr | BinOp::ShrUnchecked => None, From d5bc58634cdbf044d4f619e334c47dd03c23ed30 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 15 Dec 2024 06:58:18 +0000 Subject: [PATCH 5/5] compiletest: Deduplicate `--check-prefix` flags Currently having a revision named like `MSVC` causes errors because it gets passed via `--check-prefix` twice; once from the revision name and once from the default `msvc_or_not` value that compiletest sets. Fix this by deduplicating revision names before passing the arguments. --- src/tools/compiletest/src/runtest.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 7b11bf3b1219c..10c7ceac37ae6 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -1,5 +1,5 @@ use std::borrow::Cow; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::ffi::OsString; use std::fs::{self, File, create_dir_all}; use std::hash::{DefaultHasher, Hash, Hasher}; @@ -1939,16 +1939,18 @@ impl<'test> TestCx<'test> { // via `filecheck-flags` or by adding new header directives. // Because we use custom prefixes, we also have to register the default prefix. - filecheck.arg("--check-prefix=CHECK"); + // Deduplicate these in a set so revisions like `CHECK`, `MSVC`, and `NONMSVC` don't + // cause errors. + let mut check_prefixes = BTreeSet::from(["CHECK"]); // Some tests use the current revision name as a check prefix. if let Some(rev) = self.revision { - filecheck.arg("--check-prefix").arg(rev); + check_prefixes.insert(rev); } // Some tests also expect either the MSVC or NONMSVC prefix to be defined. let msvc_or_not = if self.config.target.contains("msvc") { "MSVC" } else { "NONMSVC" }; - filecheck.arg("--check-prefix").arg(msvc_or_not); + check_prefixes.insert(msvc_or_not); // The filecheck tool normally fails if a prefix is defined but not used. // However, we define several prefixes globally for all tests. @@ -1960,6 +1962,11 @@ impl<'test> TestCx<'test> { // Add custom flags supplied by the `filecheck-flags:` test header. filecheck.args(&self.props.filecheck_flags); + for prefix in check_prefixes { + // Set the expected prefixes + filecheck.arg("--check-prefix").arg(prefix); + } + self.compose_and_run(filecheck, "", None, None) }