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] 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 20dcb4cf34d..527a505ccc4 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 abf63e33c35..24923e5a408 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 e5ad31eb948..d847e524f8c 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 de36d6ce44d..089a5b038e3 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<types::Typ
pointer_ty(tcx)
}
}
- ty::Adt(adt_def, _) if adt_def.repr().simd() => {
- 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 03f2a65fcca..4166e763012 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<Type> {
+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 034b4e8072c..264b578c168 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 58e0a498292..622ad2ae78a 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<Value>),
}
@@ -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<Value>) {
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),