Refactoring after the PlaceValue addition

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 & docs, and change some `PlaceRef` to `PlaceValue` where the type didn't need to be included.

No behaviour changes.

Author: scottmcm

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);
                 }

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -1472,8 +1472,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         // 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);
+                        bx.typed_place_copy(scratch.val, op_place_val, op.layout);
                         (scratch.val.llval, scratch.val.align, true)
                     } else {
                         (op_place_val.llval, op_place_val.align, true)
@@ -1563,7 +1562,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);

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
             }

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);

compiler/rustc_codegen_ssa/src/mir/place.rs

@@ -15,6 +15,9 @@ 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,28 @@ impl<V: CodegenObject> PlaceValue<V> {
     pub fn new_sized(llval: V, align: Align) -> PlaceValue<V> {
         PlaceValue { llval, llextra: None, 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 +76,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,8 +85,7 @@ 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
@@ -131,18 +153,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 +213,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 +402,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>>(

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

@@ -72,8 +72,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         if val.llextra.is_some() {
                             bug!("unsized coercion on an unsized rvalue");
                         }
-                        let source = PlaceRef { val, layout: operand.layout };
-                        base::coerce_unsized_into(bx, source, dest);
+                        base::coerce_unsized_into(bx, val.with_type(operand.layout), dest);
                     }
                     OperandValue::ZeroSized => {
                         bug!("unsized coercion on a ZST rvalue");
@@ -182,10 +181,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             OperandValue::Immediate(..) | OperandValue::Pair(..) => {
                 // When we have immediate(s), the alignment of the source is irrelevant,
                 // so we can store them using the destination's alignment.
-                src.val.store(
-                    bx,
-                    PlaceRef::new_sized_aligned(dst.val.llval, src.layout, dst.val.align),
-                );
+                src.val.store(bx, dst.val.with_type(src.layout));
             }
         }
     }
@@ -223,8 +219,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             OperandValue::Ref(source_place_val) => {
                 debug_assert_eq!(source_place_val.llextra, None);
                 debug_assert!(matches!(operand_kind, OperandValueKind::Ref));
-                let fake_place = PlaceRef { val: source_place_val, layout: cast };
-                Some(bx.load_operand(fake_place).val)
+                Some(bx.load_operand(source_place_val.with_type(cast)).val)
             }
             OperandValue::ZeroSized => {
                 let OperandValueKind::ZeroSized = operand_kind else {
@@ -452,23 +447,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     }
                     mir::CastKind::PointerCoercion(PointerCoercion::Unsize) => {
                         assert!(bx.cx().is_backend_scalar_pair(cast));
-                        let (lldata, llextra) = match operand.val {
-                            OperandValue::Pair(lldata, llextra) => {
-                                // unsize from a fat pointer -- this is a
-                                // "trait-object-to-supertrait" coercion.
-                                (lldata, Some(llextra))
-                            }
-                            OperandValue::Immediate(lldata) => {
-                                // "standard" unsize
-                                (lldata, None)
-                            }
-                            OperandValue::Ref(..) => {
-                                bug!("by-ref operand {:?} in `codegen_rvalue_operand`", operand);
-                            }
-                            OperandValue::ZeroSized => {
-                                bug!("zero-sized operand {:?} in `codegen_rvalue_operand`", operand);
-                            }
-                        };
+                        let (lldata, llextra) = operand.val.pointer_parts();
                         let (lldata, llextra) =
                             base::unsize_ptr(bx, lldata, operand.layout.ty, cast.ty, llextra);
                         OperandValue::Pair(lldata, llextra)
@@ -489,12 +468,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         }
                     }
                     mir::CastKind::DynStar => {
-                        let (lldata, llextra) = match operand.val {
-                            OperandValue::Ref(..) => todo!(),
-                            OperandValue::Immediate(v) => (v, None),
-                            OperandValue::Pair(v, l) => (v, Some(l)),
-                            OperandValue::ZeroSized => bug!("ZST -- which is not PointerLike -- in DynStar"),
-                        };
+                        let (lldata, llextra) = operand.val.pointer_parts();
                         let (lldata, llextra) =
                             base::cast_to_dyn_star(bx, lldata, operand.layout, cast.ty, llextra);
                         OperandValue::Pair(lldata, llextra)
@@ -762,16 +736,18 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         mk_ptr_ty: impl FnOnce(TyCtxt<'tcx>, Ty<'tcx>) -> Ty<'tcx>,
     ) -> OperandRef<'tcx, Bx::Value> {
         let cg_place = self.codegen_place(bx, place.as_ref());
+        let val = cg_place.val.address();
 
         let ty = cg_place.layout.ty;
+        debug_assert!(
+            if bx.cx().type_has_metadata(ty) {
+                matches!(val, OperandValue::Pair(..))
+            } else {
+                matches!(val, OperandValue::Immediate(..))
+            },
+            "Address of place was unexpectedly {val:?} for pointee type {ty:?}",
+        );
 
-        // Note: places are indirect, so storing the `llval` into the
-        // destination effectively creates a reference.
-        let val = if !bx.cx().type_has_metadata(ty) {
-            OperandValue::Immediate(cg_place.val.llval)
-        } else {
-            OperandValue::Pair(cg_place.val.llval, cg_place.val.llextra.unwrap())
-        };
         OperandRef { val, layout: self.cx.layout_of(mk_ptr_ty(self.cx.tcx(), ty)) }
     }