From eed1f754151965c4f9f7eef877801043101b83e1 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 25 Mar 2023 14:30:12 +0000 Subject: [PATCH 1/2] Don't store vector types in ssa variables They have been causing a lot of trouble. For example current MIR building thinks that it is fine to dynamically index into them. And there are different paths depending on if the repr(simd) struct uses fields or a single array as interior. There is also trouble with moving the inner array of a repr(simd) type that is an array wrapper. If performance becomes a concern, I will implement this in a more principled way. --- scripts/test_rustc_tests.sh | 1 - src/abi/comments.rs | 1 - src/abi/pass_mode.rs | 4 +- src/common.rs | 20 +---- src/intrinsics/mod.rs | 8 +- src/intrinsics/simd.rs | 2 +- src/value_and_place.rs | 146 +++--------------------------------- 7 files changed, 15 insertions(+), 167 deletions(-) diff --git a/scripts/test_rustc_tests.sh b/scripts/test_rustc_tests.sh index 20dcb4cf34d2f..527a505ccc4ee 100755 --- a/scripts/test_rustc_tests.sh +++ b/scripts/test_rustc_tests.sh @@ -112,7 +112,6 @@ rm tests/incremental/spike-neg2.rs # same rm tests/ui/simd/intrinsic/generic-reduction-pass.rs # simd_reduce_add_unordered doesn't accept an accumulator for integer vectors -rm tests/ui/simd/intrinsic/generic-as.rs # crash when accessing vector type field (#1318) rm tests/ui/simd/simd-bitmask.rs # crash # bugs in the test suite diff --git a/src/abi/comments.rs b/src/abi/comments.rs index abf63e33c3537..24923e5a4089e 100644 --- a/src/abi/comments.rs +++ b/src/abi/comments.rs @@ -100,7 +100,6 @@ pub(super) fn add_local_place_comments<'tcx>( assert_eq!(local, place_local); ("ssa", Cow::Owned(format!("var=({}, {})", var1.index(), var2.index()))) } - CPlaceInner::VarLane(_local, _var, _lane) => unreachable!(), CPlaceInner::Addr(ptr, meta) => { let meta = if let Some(meta) = meta { Cow::Owned(format!("meta={}", meta)) diff --git a/src/abi/pass_mode.rs b/src/abi/pass_mode.rs index e5ad31eb9484a..d847e524f8cfa 100644 --- a/src/abi/pass_mode.rs +++ b/src/abi/pass_mode.rs @@ -84,7 +84,7 @@ impl<'tcx> ArgAbiExt<'tcx> for ArgAbi<'tcx, Ty<'tcx>> { attrs )], Abi::Vector { .. } => { - let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout).unwrap(); + let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout); smallvec![AbiParam::new(vector_ty)] } _ => unreachable!("{:?}", self.layout.abi), @@ -135,7 +135,7 @@ impl<'tcx> ArgAbiExt<'tcx> for ArgAbi<'tcx, Ty<'tcx>> { (None, vec![AbiParam::new(scalar_to_clif_type(tcx, scalar))]) } Abi::Vector { .. } => { - let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout).unwrap(); + let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout); (None, vec![AbiParam::new(vector_ty)]) } _ => unreachable!("{:?}", self.layout.abi), diff --git a/src/common.rs b/src/common.rs index de36d6ce44dc2..089a5b038e393 100644 --- a/src/common.rs +++ b/src/common.rs @@ -72,19 +72,6 @@ fn clif_type_from_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option { - let (element, count) = match &tcx.layout_of(ParamEnv::reveal_all().and(ty)).unwrap().abi - { - Abi::Vector { element, count } => (*element, *count), - _ => unreachable!(), - }; - - match scalar_to_clif_type(tcx, element).by(u32::try_from(count).unwrap()) { - // Cranelift currently only implements icmp for 128bit vectors. - Some(vector_ty) if vector_ty.bits() == 128 => vector_ty, - _ => return None, - } - } ty::Param(_) => bug!("ty param {:?}", ty), _ => return None, }) @@ -96,12 +83,7 @@ fn clif_pair_type_from_ty<'tcx>( ) -> Option<(types::Type, types::Type)> { Some(match ty.kind() { ty::Tuple(types) if types.len() == 2 => { - let a = clif_type_from_ty(tcx, types[0])?; - let b = clif_type_from_ty(tcx, types[1])?; - if a.is_vector() || b.is_vector() { - return None; - } - (a, b) + (clif_type_from_ty(tcx, types[0])?, clif_type_from_ty(tcx, types[1])?) } ty::RawPtr(TypeAndMut { ty: pointee_ty, mutbl: _ }) | ty::Ref(_, pointee_ty, _) => { if has_ptr_meta(tcx, *pointee_ty) { diff --git a/src/intrinsics/mod.rs b/src/intrinsics/mod.rs index 03f2a65fccad2..4166e763012ab 100644 --- a/src/intrinsics/mod.rs +++ b/src/intrinsics/mod.rs @@ -51,17 +51,13 @@ fn report_atomic_type_validation_error<'tcx>( fx.bcx.ins().trap(TrapCode::UnreachableCodeReached); } -pub(crate) fn clif_vector_type<'tcx>(tcx: TyCtxt<'tcx>, layout: TyAndLayout<'tcx>) -> Option { +pub(crate) fn clif_vector_type<'tcx>(tcx: TyCtxt<'tcx>, layout: TyAndLayout<'tcx>) -> Type { let (element, count) = match layout.abi { Abi::Vector { element, count } => (element, count), _ => unreachable!(), }; - match scalar_to_clif_type(tcx, element).by(u32::try_from(count).unwrap()) { - // Cranelift currently only implements icmp for 128bit vectors. - Some(vector_ty) if vector_ty.bits() == 128 => Some(vector_ty), - _ => None, - } + scalar_to_clif_type(tcx, element).by(u32::try_from(count).unwrap()).unwrap() } fn simd_for_each_lane<'tcx>( diff --git a/src/intrinsics/simd.rs b/src/intrinsics/simd.rs index 034b4e8072cc5..264b578c168b2 100644 --- a/src/intrinsics/simd.rs +++ b/src/intrinsics/simd.rs @@ -253,7 +253,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>( } ret.write_cvalue(fx, base); - let ret_lane = ret.place_field(fx, mir::Field::new(idx.try_into().unwrap())); + let ret_lane = ret.place_lane(fx, idx.try_into().unwrap()); ret_lane.write_cvalue(fx, val); } diff --git a/src/value_and_place.rs b/src/value_and_place.rs index 58e0a498292d4..622ad2ae78a25 100644 --- a/src/value_and_place.rs +++ b/src/value_and_place.rs @@ -3,7 +3,6 @@ use crate::prelude::*; use cranelift_codegen::ir::immediates::Offset32; -use cranelift_codegen::ir::{InstructionData, Opcode}; fn codegen_field<'tcx>( fx: &mut FunctionCx<'_, '_, 'tcx>, @@ -166,9 +165,6 @@ impl<'tcx> CValue<'tcx> { CValueInner::ByRef(ptr, None) => { let clif_ty = match layout.abi { Abi::Scalar(scalar) => scalar_to_clif_type(fx.tcx, scalar), - Abi::Vector { element, count } => scalar_to_clif_type(fx.tcx, element) - .by(u32::try_from(count).unwrap()) - .unwrap(), _ => unreachable!("{:?}", layout.ty), }; let mut flags = MemFlags::new(); @@ -214,17 +210,7 @@ impl<'tcx> CValue<'tcx> { ) -> CValue<'tcx> { let layout = self.1; match self.0 { - CValueInner::ByVal(val) => match layout.abi { - Abi::Vector { element: _, count } => { - let count = u8::try_from(count).expect("SIMD type with more than 255 lanes???"); - let field = u8::try_from(field.index()).unwrap(); - assert!(field < count); - let lane = fx.bcx.ins().extractlane(val, field); - let field_layout = layout.field(&*fx, usize::from(field)); - CValue::by_val(lane, field_layout) - } - _ => unreachable!("value_field for ByVal with abi {:?}", layout.abi), - }, + CValueInner::ByVal(_) => unreachable!(), CValueInner::ByValPair(val1, val2) => match layout.abi { Abi::ScalarPair(_, _) => { let val = match field.as_u32() { @@ -258,16 +244,7 @@ impl<'tcx> CValue<'tcx> { let lane_layout = fx.layout_of(lane_ty); assert!(lane_idx < lane_count); match self.0 { - CValueInner::ByVal(val) => match layout.abi { - Abi::Vector { element: _, count: _ } => { - assert!(lane_count <= u8::MAX.into(), "SIMD type with more than 255 lanes???"); - let lane_idx = u8::try_from(lane_idx).unwrap(); - let lane = fx.bcx.ins().extractlane(val, lane_idx); - CValue::by_val(lane, lane_layout) - } - _ => unreachable!("value_lane for ByVal with abi {:?}", layout.abi), - }, - CValueInner::ByValPair(_, _) => unreachable!(), + CValueInner::ByVal(_) | CValueInner::ByValPair(_, _) => unreachable!(), CValueInner::ByRef(ptr, None) => { let field_offset = lane_layout.size * lane_idx; let field_ptr = ptr.offset_i64(fx, i64::try_from(field_offset.bytes()).unwrap()); @@ -348,7 +325,6 @@ pub(crate) struct CPlace<'tcx> { pub(crate) enum CPlaceInner { Var(Local, Variable), VarPair(Local, Variable, Variable), - VarLane(Local, Variable, u8), Addr(Pointer, Option), } @@ -442,12 +418,6 @@ impl<'tcx> CPlace<'tcx> { //fx.bcx.set_val_label(val2, cranelift_codegen::ir::ValueLabel::new(var2.index())); CValue::by_val_pair(val1, val2, layout) } - CPlaceInner::VarLane(_local, var, lane) => { - let val = fx.bcx.use_var(var); - //fx.bcx.set_val_label(val, cranelift_codegen::ir::ValueLabel::new(var.index())); - let val = fx.bcx.ins().extractlane(val, lane); - CValue::by_val(val, layout) - } CPlaceInner::Addr(ptr, extra) => { if let Some(extra) = extra { CValue::by_ref_unsized(ptr, extra, layout) @@ -470,9 +440,9 @@ impl<'tcx> CPlace<'tcx> { pub(crate) fn to_ptr_maybe_unsized(self) -> (Pointer, Option) { match self.inner { CPlaceInner::Addr(ptr, extra) => (ptr, extra), - CPlaceInner::Var(_, _) - | CPlaceInner::VarPair(_, _, _) - | CPlaceInner::VarLane(_, _, _) => bug!("Expected CPlace::Addr, found {:?}", self), + CPlaceInner::Var(_, _) | CPlaceInner::VarPair(_, _, _) => { + bug!("Expected CPlace::Addr, found {:?}", self) + } } } @@ -565,26 +535,6 @@ impl<'tcx> CPlace<'tcx> { let dst_layout = self.layout(); let to_ptr = match self.inner { CPlaceInner::Var(_local, var) => { - if let ty::Array(element, len) = dst_layout.ty.kind() { - // Can only happen for vector types - let len = u32::try_from(len.eval_target_usize(fx.tcx, ParamEnv::reveal_all())) - .unwrap(); - let vector_ty = fx.clif_type(*element).unwrap().by(len).unwrap(); - - let data = match from.0 { - CValueInner::ByRef(ptr, None) => { - let mut flags = MemFlags::new(); - flags.set_notrap(); - ptr.load(fx, vector_ty, flags) - } - CValueInner::ByVal(_) - | CValueInner::ByValPair(_, _) - | CValueInner::ByRef(_, Some(_)) => bug!("array should be ByRef"), - }; - - fx.bcx.def_var(var, data); - return; - } let data = CValue(from.0, dst_layout).load_scalar(fx); let dst_ty = fx.clif_type(self.layout().ty).unwrap(); transmute_value(fx, var, data, dst_ty); @@ -603,22 +553,6 @@ impl<'tcx> CPlace<'tcx> { transmute_value(fx, var2, data2, dst_ty2); return; } - CPlaceInner::VarLane(_local, var, lane) => { - let data = from.load_scalar(fx); - - // First get the old vector - let vector = fx.bcx.use_var(var); - //fx.bcx.set_val_label(vector, cranelift_codegen::ir::ValueLabel::new(var.index())); - - // Next insert the written lane into the vector - let vector = fx.bcx.ins().insertlane(vector, data, lane); - - // Finally write the new vector - //fx.bcx.set_val_label(vector, cranelift_codegen::ir::ValueLabel::new(var.index())); - fx.bcx.def_var(var, vector); - - return; - } CPlaceInner::Addr(ptr, None) => { if dst_layout.size == Size::ZERO || dst_layout.abi == Abi::Uninhabited { return; @@ -631,7 +565,6 @@ impl<'tcx> CPlace<'tcx> { let mut flags = MemFlags::new(); flags.set_notrap(); match from.layout().abi { - // FIXME make Abi::Vector work too Abi::Scalar(_) => { let val = from.load_scalar(fx); to_ptr.store(fx, val, flags); @@ -692,39 +625,6 @@ impl<'tcx> CPlace<'tcx> { let layout = self.layout(); match self.inner { - CPlaceInner::Var(local, var) => match layout.ty.kind() { - ty::Array(_, _) => { - // Can only happen for vector types - return CPlace { - inner: CPlaceInner::VarLane(local, var, field.as_u32().try_into().unwrap()), - layout: layout.field(fx, field.as_u32().try_into().unwrap()), - }; - } - ty::Adt(adt_def, substs) if layout.ty.is_simd() => { - let f0_ty = adt_def.non_enum_variant().fields[0].ty(fx.tcx, substs); - - match f0_ty.kind() { - ty::Array(_, _) => { - assert_eq!(field.as_u32(), 0); - return CPlace { - inner: CPlaceInner::Var(local, var), - layout: layout.field(fx, field.as_u32().try_into().unwrap()), - }; - } - _ => { - return CPlace { - inner: CPlaceInner::VarLane( - local, - var, - field.as_u32().try_into().unwrap(), - ), - layout: layout.field(fx, field.as_u32().try_into().unwrap()), - }; - } - } - } - _ => {} - }, CPlaceInner::VarPair(local, var1, var2) => { let layout = layout.field(&*fx, field.index()); @@ -766,15 +666,8 @@ impl<'tcx> CPlace<'tcx> { assert!(lane_idx < lane_count); match self.inner { - CPlaceInner::Var(local, var) => { - assert!(matches!(layout.abi, Abi::Vector { .. })); - CPlace { - inner: CPlaceInner::VarLane(local, var, lane_idx.try_into().unwrap()), - layout: lane_layout, - } - } + CPlaceInner::Var(_, _) => unreachable!(), CPlaceInner::VarPair(_, _, _) => unreachable!(), - CPlaceInner::VarLane(_, _, _) => unreachable!(), CPlaceInner::Addr(ptr, None) => { let field_offset = lane_layout.size * lane_idx; let field_ptr = ptr.offset_i64(fx, i64::try_from(field_offset.bytes()).unwrap()); @@ -793,32 +686,11 @@ impl<'tcx> CPlace<'tcx> { ty::Array(elem_ty, _) => { let elem_layout = fx.layout_of(*elem_ty); match self.inner { - CPlaceInner::Var(local, var) => { - // This is a hack to handle `vector_val.0[1]`. It doesn't allow dynamic - // indexing. - let lane_idx = match fx.bcx.func.dfg.insts - [fx.bcx.func.dfg.value_def(index).unwrap_inst()] - { - InstructionData::UnaryImm { opcode: Opcode::Iconst, imm } => imm, - _ => bug!( - "Dynamic indexing into a vector type is not supported: {self:?}[{index}]" - ), - }; - return CPlace { - inner: CPlaceInner::VarLane( - local, - var, - lane_idx.bits().try_into().unwrap(), - ), - layout: elem_layout, - }; - } CPlaceInner::Addr(addr, None) => (elem_layout, addr), - CPlaceInner::Addr(_, Some(_)) - | CPlaceInner::VarPair(_, _, _) - | CPlaceInner::VarLane(_, _, _) => bug!("Can't index into {self:?}"), + CPlaceInner::Var(_, _) + | CPlaceInner::Addr(_, Some(_)) + | CPlaceInner::VarPair(_, _, _) => bug!("Can't index into {self:?}"), } - // FIXME use VarLane in case of Var with simd type } ty::Slice(elem_ty) => (fx.layout_of(*elem_ty), self.to_ptr_maybe_unsized().0), _ => bug!("place_index({:?})", self.layout().ty), From c3ee03011962d53f44d9d0faf26260f914191cfc Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sun, 26 Mar 2023 10:22:37 +0000 Subject: [PATCH 2/2] Fix passing and returning vector types --- example/std_example.rs | 10 +++++++++- src/value_and_place.rs | 3 +++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/example/std_example.rs b/example/std_example.rs index c86063f60b94d..ab4045d11a663 100644 --- a/example/std_example.rs +++ b/example/std_example.rs @@ -1,4 +1,4 @@ -#![feature(core_intrinsics, generators, generator_trait, is_sorted)] +#![feature(core_intrinsics, generators, generator_trait, is_sorted, repr_simd)] #[cfg(target_arch = "x86_64")] use std::arch::x86_64::*; @@ -153,12 +153,20 @@ fn main() { enum Never {} } + + foo(I64X2(0, 0)); } fn panic(_: u128) { panic!(); } +#[repr(simd)] +struct I64X2(i64, i64); + +#[allow(improper_ctypes_definitions)] +extern "C" fn foo(_a: I64X2) {} + #[cfg(target_arch = "x86_64")] #[target_feature(enable = "sse2")] unsafe fn test_simd() { diff --git a/src/value_and_place.rs b/src/value_and_place.rs index 622ad2ae78a25..cd24106257a74 100644 --- a/src/value_and_place.rs +++ b/src/value_and_place.rs @@ -165,6 +165,9 @@ impl<'tcx> CValue<'tcx> { CValueInner::ByRef(ptr, None) => { let clif_ty = match layout.abi { Abi::Scalar(scalar) => scalar_to_clif_type(fx.tcx, scalar), + Abi::Vector { element, count } => scalar_to_clif_type(fx.tcx, element) + .by(u32::try_from(count).unwrap()) + .unwrap(), _ => unreachable!("{:?}", layout.ty), }; let mut flags = MemFlags::new();