Refactoring after the PlaceValue addition

scottmcm · scottmcm · commit a9ef67f294a8 · 2024-04-24T00:41:11.000-07:00
I added `PlaceValue` in 123775, but kept that one line-by-line simple because it touched so many places.

This goes through to add more helpers &amp; docs, and change some `PlaceRef` to `PlaceValue` where the type didn't need to be included.

No behaviour changes.
diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs
@@ -282,7 +282,7 @@ pub fn coerce_unsized_into<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
                 }
 
                 if src_f.layout.ty == dst_f.layout.ty {
-                    bx.typed_place_copy(dst_f, src_f);
+                    bx.typed_place_copy(dst_f.val, src_f.val, src_f.layout);
                 } else {
                     coerce_unsized_into(bx, src_f, dst_f);
                 }
diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs
@@ -1453,9 +1453,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         Some(pointee_align) => cmp::max(pointee_align, arg.layout.align.abi),
                         None => arg.layout.align.abi,
                     };
-                    let scratch = PlaceRef::alloca_aligned(bx, arg.layout, required_align);
-                    op.val.store(bx, scratch);
-                    (scratch.val.llval, scratch.val.align, true)
+                    let scratch = PlaceValue::alloca(bx, arg.layout.size, required_align);
+                    op.val.store(bx, scratch.with_type(arg.layout));
+                    (scratch.llval, scratch.align, true)
                 }
                 PassMode::Cast { .. } => {
                     let scratch = PlaceRef::alloca(bx, arg.layout);
@@ -1474,10 +1474,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         // For `foo(packed.large_field)`, and types with <4 byte alignment on x86,
                         // alignment requirements may be higher than the type's alignment, so copy
                         // to a higher-aligned alloca.
-                        let scratch = PlaceRef::alloca_aligned(bx, arg.layout, required_align);
-                        let op_place = PlaceRef { val: op_place_val, layout: op.layout };
-                        bx.typed_place_copy(scratch, op_place);
-                        (scratch.val.llval, scratch.val.align, true)
+                        let scratch = PlaceValue::alloca(bx, arg.layout.size, required_align);
+                        bx.typed_place_copy(scratch, op_place_val, op.layout);
+                        (scratch.llval, scratch.align, true)
                     } else {
                         (op_place_val.llval, op_place_val.align, true)
                     }
@@ -1566,7 +1565,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             if place_val.llextra.is_some() {
                 bug!("closure arguments must be sized");
             }
-            let tuple_ptr = PlaceRef { val: place_val, layout: tuple.layout };
+            let tuple_ptr = place_val.with_type(tuple.layout);
             for i in 0..tuple.layout.fields.count() {
                 let field_ptr = tuple_ptr.project_field(bx, i);
                 let field = bx.load_operand(field_ptr);
diff --git a/compiler/rustc_codegen_ssa/src/mir/intrinsic.rs b/compiler/rustc_codegen_ssa/src/mir/intrinsic.rs
@@ -1,4 +1,4 @@
-use super::operand::{OperandRef, OperandValue};
+use super::operand::OperandRef;
 use super::place::PlaceRef;
 use super::FunctionCx;
 use crate::errors;
@@ -92,9 +92,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 // into the (unoptimized) direct swapping implementation, so we disable it.
                 || bx.sess().target.arch == "spirv"
             {
-                let x_place = PlaceRef::new_sized(args[0].immediate(), pointee_layout);
-                let y_place = PlaceRef::new_sized(args[1].immediate(), pointee_layout);
-                bx.typed_place_swap(x_place, y_place);
+                let align = pointee_layout.align.abi;
+                let x_place = args[0].val.deref(align);
+                let y_place = args[1].val.deref(align);
+                bx.typed_place_swap(x_place, y_place, pointee_layout);
                 return Ok(());
             }
         }
@@ -112,15 +113,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             sym::va_end => bx.va_end(args[0].immediate()),
             sym::size_of_val => {
                 let tp_ty = fn_args.type_at(0);
-                let meta =
-                    if let OperandValue::Pair(_, meta) = args[0].val { Some(meta) } else { None };
+                let (_, meta) = args[0].val.pointer_parts();
                 let (llsize, _) = size_of_val::size_and_align_of_dst(bx, tp_ty, meta);
                 llsize
             }
             sym::min_align_of_val => {
                 let tp_ty = fn_args.type_at(0);
-                let meta =
-                    if let OperandValue::Pair(_, meta) = args[0].val { Some(meta) } else { None };
+                let (_, meta) = args[0].val.pointer_parts();
                 let (_, llalign) = size_of_val::size_and_align_of_dst(bx, tp_ty, meta);
                 llalign
             }
diff --git a/compiler/rustc_codegen_ssa/src/mir/operand.rs b/compiler/rustc_codegen_ssa/src/mir/operand.rs
@@ -57,6 +57,32 @@ pub enum OperandValue<V> {
     ZeroSized,
 }
 
+impl<V: CodegenObject> OperandValue<V> {
+    /// Treat this value as a pointer and return the data pointer and
+    /// optional metadata as backend values.
+    ///
+    /// If you're making a place, use [`Self::deref`] instead.
+    pub fn pointer_parts(self) -> (V, Option<V>) {
+        match self {
+            OperandValue::Immediate(llptr) => (llptr, None),
+            OperandValue::Pair(llptr, llextra) => (llptr, Some(llextra)),
+            _ => bug!("OperandValue cannot be a pointer: {self:?}"),
+        }
+    }
+
+    /// Treat this value as a pointer and return the place to which it points.
+    ///
+    /// The pointer immediate doesn't inherently know its alignment,
+    /// so you need to pass it in. If you want to get it from a type's ABI
+    /// alignment, then maybe you want [`OperandRef::deref`] instead.
+    ///
+    /// This is the inverse of [`PlaceValue::address`].
+    pub fn deref(self, align: Align) -> PlaceValue<V> {
+        let (llval, llextra) = self.pointer_parts();
+        PlaceValue { llval, llextra, align }
+    }
+}
+
 /// An `OperandRef` is an "SSA" reference to a Rust value, along with
 /// its type.
 ///
@@ -204,6 +230,15 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
         }
     }
 
+    /// Asserts that this operand is a pointer (or reference) and returns
+    /// the place to which it points.  (This requires no code to be emitted
+    /// as we represent places using the pointer to the place.)
+    ///
+    /// This uses [`Ty::builtin_deref`] to include the type of the place and
+    /// assumes the place is aligned to the pointee's usual ABI alignment.
+    ///
+    /// If you don't need the type, see [`OperandValue::pointer_parts`]
+    /// or [`OperandValue::deref`].
     pub fn deref<Cx: LayoutTypeMethods<'tcx>>(self, cx: &Cx) -> PlaceRef<'tcx, V> {
         if self.layout.ty.is_box() {
             // Derefer should have removed all Box derefs
@@ -217,15 +252,8 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
             .unwrap_or_else(|| bug!("deref of non-pointer {:?}", self))
             .ty;
 
-        let (llptr, llextra) = match self.val {
-            OperandValue::Immediate(llptr) => (llptr, None),
-            OperandValue::Pair(llptr, llextra) => (llptr, Some(llextra)),
-            OperandValue::Ref(..) => bug!("Deref of by-Ref operand {:?}", self),
-            OperandValue::ZeroSized => bug!("Deref of ZST operand {:?}", self),
-        };
         let layout = cx.layout_of(projected_ty);
-        let val = PlaceValue { llval: llptr, llextra, align: layout.align.abi };
-        PlaceRef { val, layout }
+        self.val.deref(layout.align.abi).with_type(layout)
     }
 
     /// If this operand is a `Pair`, we return an aggregate with the two values.
@@ -418,8 +446,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> {
                 if val.llextra.is_some() {
                     bug!("cannot directly store unsized values");
                 }
-                let source_place = PlaceRef { val, layout: dest.layout };
-                bx.typed_place_copy_with_flags(dest, source_place, flags);
+                bx.typed_place_copy_with_flags(dest.val, val, dest.layout, flags);
             }
             OperandValue::Immediate(s) => {
                 let val = bx.from_immediate(s);
diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs
@@ -9,12 +9,15 @@ use rustc_middle::mir;
 use rustc_middle::mir::tcx::PlaceTy;
 use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, Ty};
-use rustc_target::abi::{Align, FieldsShape, Int, Pointer, TagEncoding};
+use rustc_target::abi::{Align, FieldsShape, Int, Pointer, Size, TagEncoding};
 use rustc_target::abi::{VariantIdx, Variants};
 
 /// The location and extra runtime properties of the place.
 ///
 /// Typically found in a [`PlaceRef`] or an [`OperandValue::Ref`].
+///
+/// As a location in memory, this has no specific type. If you want to
+/// load or store it using a typed operation, use [`Self::with_type`].
 #[derive(Copy, Clone, Debug)]
 pub struct PlaceValue<V> {
     /// A pointer to the contents of the place.
@@ -34,6 +37,41 @@ impl<V: CodegenObject> PlaceValue<V> {
     pub fn new_sized(llval: V, align: Align) -> PlaceValue<V> {
         PlaceValue { llval, llextra: None, align }
     }
+
+    /// Allocates a stack slot in the function for a value
+    /// of the specified size and alignment.
+    ///
+    /// The allocation itself is untyped.
+    pub fn alloca<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx, Value = V>>(
+        bx: &mut Bx,
+        size: Size,
+        align: Align,
+    ) -> PlaceValue<V> {
+        let llval = bx.alloca(size, align);
+        PlaceValue::new_sized(llval, align)
+    }
+
+    /// Creates a `PlaceRef` to this location with the given type.
+    pub fn with_type<'tcx>(self, layout: TyAndLayout<'tcx>) -> PlaceRef<'tcx, V> {
+        debug_assert!(
+            layout.is_unsized() || layout.abi.is_uninhabited() || self.llextra.is_none(),
+            "Had pointer metadata {:?} for sized type {layout:?}",
+            self.llextra,
+        );
+        PlaceRef { val: self, layout }
+    }
+
+    /// Gets the pointer to this place as an [`OperandValue::Immediate`]
+    /// or, for those needing metadata, an [`OperandValue::Pair`].
+    ///
+    /// This is the inverse of [`OperandValue::deref`].
+    pub fn address(self) -> OperandValue<V> {
+        if let Some(llextra) = self.llextra {
+            OperandValue::Pair(self.llval, llextra)
+        } else {
+            OperandValue::Immediate(self.llval)
+        }
+    }
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -51,9 +89,7 @@ pub struct PlaceRef<'tcx, V> {
 
 impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
     pub fn new_sized(llval: V, layout: TyAndLayout<'tcx>) -> PlaceRef<'tcx, V> {
-        assert!(layout.is_sized());
-        let val = PlaceValue::new_sized(llval, layout.align.abi);
-        PlaceRef { val, layout }
+        PlaceRef::new_sized_aligned(llval, layout, layout.align.abi)
     }
 
     pub fn new_sized_aligned(
@@ -62,27 +98,17 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
         align: Align,
     ) -> PlaceRef<'tcx, V> {
         assert!(layout.is_sized());
-        let val = PlaceValue::new_sized(llval, align);
-        PlaceRef { val, layout }
+        PlaceValue::new_sized(llval, align).with_type(layout)
     }
 
     // FIXME(eddyb) pass something else for the name so no work is done
     // unless LLVM IR names are turned on (e.g. for `--emit=llvm-ir`).
     pub fn alloca<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
         bx: &mut Bx,
         layout: TyAndLayout<'tcx>,
-    ) -> Self {
-        Self::alloca_aligned(bx, layout, layout.align.abi)
-    }
-
-    pub fn alloca_aligned<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
-        bx: &mut Bx,
-        layout: TyAndLayout<'tcx>,
-        align: Align,
     ) -> Self {
         assert!(layout.is_sized(), "tried to statically allocate unsized place");
-        let tmp = bx.alloca(layout.size, align);
-        Self::new_sized_aligned(tmp, layout, align)
+        PlaceValue::alloca(bx, layout.size, layout.align.abi).with_type(layout)
     }
 
     /// Returns a place for an indirect reference to an unsized place.
@@ -131,18 +157,12 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
             } else {
                 bx.inbounds_ptradd(self.val.llval, bx.const_usize(offset.bytes()))
             };
-            PlaceRef {
-                val: PlaceValue {
-                    llval,
-                    llextra: if bx.cx().type_has_metadata(field.ty) {
-                        self.val.llextra
-                    } else {
-                        None
-                    },
-                    align: effective_field_align,
-                },
-                layout: field,
-            }
+            let val = PlaceValue {
+                llval,
+                llextra: if bx.cx().type_has_metadata(field.ty) { self.val.llextra } else { None },
+                align: effective_field_align,
+            };
+            val.with_type(field)
         };
 
         // Simple cases, which don't need DST adjustment:
@@ -197,7 +217,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
         let ptr = bx.inbounds_ptradd(self.val.llval, offset);
         let val =
             PlaceValue { llval: ptr, llextra: self.val.llextra, align: effective_field_align };
-        PlaceRef { val, layout: field }
+        val.with_type(field)
     }
 
     /// Obtain the actual discriminant of a value.
@@ -386,18 +406,13 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
             layout.size
         };
 
-        PlaceRef {
-            val: PlaceValue {
-                llval: bx.inbounds_gep(
-                    bx.cx().backend_type(self.layout),
-                    self.val.llval,
-                    &[bx.cx().const_usize(0), llindex],
-                ),
-                llextra: None,
-                align: self.val.align.restrict_for_offset(offset),
-            },
-            layout,
-        }
+        let llval = bx.inbounds_gep(
+            bx.cx().backend_type(self.layout),
+            self.val.llval,
+            &[bx.cx().const_usize(0), llindex],
+        );
+        let align = self.val.align.restrict_for_offset(offset);
+        PlaceValue::new_sized(llval, align).with_type(layout)
     }
 
     pub fn project_downcast<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs
diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs

Original file line number	Diff line number	Diff line change
`@@ -282,7 +282,7 @@ pub fn coerce_unsized_into<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(`
`282`	`282`	`}`
`283`	`283`
`284`	`284`	`if src_f.layout.ty == dst_f.layout.ty {`
`285`		`- bx.typed_place_copy(dst_f, src_f);`
	`285`	`+ bx.typed_place_copy(dst_f.val, src_f.val, src_f.layout);`
`286`	`286`	`} else {`
`287`	`287`	`coerce_unsized_into(bx, src_f, dst_f);`
`288`	`288`	`}`