Don't store vector types in ssa variables

bjorn3 · bjorn3 · commit eed1f7541519 · 2023-03-25T14:30:12.000Z
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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),

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,6 @@ pub(super) fn add_local_place_comments<'tcx>(`
`100`	`100`	`assert_eq!(local, place_local);`
`101`	`101`	`("ssa", Cow::Owned(format!("var=({}, {})", var1.index(), var2.index())))`
`102`	`102`	`}`
`103`		`- CPlaceInner::VarLane(_local, _var, _lane) => unreachable!(),`
`104`	`103`	`CPlaceInner::Addr(ptr, meta) => {`
`105`	`104`	`let meta = if let Some(meta) = meta {`
`106`	`105`	`Cow::Owned(format!("meta={}", meta))`
Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ impl<'tcx> ArgAbiExt<'tcx> for ArgAbi<'tcx, Ty<'tcx>> {`
`84`	`84`	`attrs`
`85`	`85`	`)],`
`86`	`86`	`Abi::Vector { .. } => {`
`87`		`- let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout).unwrap();`
	`87`	`+ let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout);`
`88`	`88`	`smallvec![AbiParam::new(vector_ty)]`
`89`	`89`	`}`
`90`	`90`	`_ => unreachable!("{:?}", self.layout.abi),`
`@@ -135,7 +135,7 @@ impl<'tcx> ArgAbiExt<'tcx> for ArgAbi<'tcx, Ty<'tcx>> {`
`135`	`135`	`(None, vec![AbiParam::new(scalar_to_clif_type(tcx, scalar))])`
`136`	`136`	`}`
`137`	`137`	`Abi::Vector { .. } => {`
`138`		`- let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout).unwrap();`
	`138`	`+ let vector_ty = crate::intrinsics::clif_vector_type(tcx, self.layout);`
`139`	`139`	`(None, vec![AbiParam::new(vector_ty)])`
`140`	`140`	`}`
`141`	`141`	`_ => unreachable!("{:?}", self.layout.abi),`
Original file line number	Diff line number	Diff line change
`@@ -253,7 +253,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(`
`253`	`253`	`}`
`254`	`254`
`255`	`255`	`ret.write_cvalue(fx, base);`
`256`		`- let ret_lane = ret.place_field(fx, mir::Field::new(idx.try_into().unwrap()));`
	`256`	`+ let ret_lane = ret.place_lane(fx, idx.try_into().unwrap());`
`257`	`257`	`ret_lane.write_cvalue(fx, val);`
`258`	`258`	`}`
`259`	`259`