Skip to content

Commit

Permalink
Add a distinct OperandValue::ZeroSized variant for ZSTs
Browse files Browse the repository at this point in the history
These tend to have special handling in a bunch of places anyway, so the variant helps remember that.  And I think it's easier to grok than non-Scalar Aggregates sometimes being `Immediates` (like I got wrong and caused 109992).  As a minor bonus, it means we don't need to generate poison LLVM values for them to pass around in `OperandValue::Immediate`s.
  • Loading branch information
scottmcm committed May 29, 2023
1 parent dc0943d commit 9495095
Show file tree
Hide file tree
Showing 11 changed files with 167 additions and 86 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> {
assert_eq!(place.llextra.is_some(), place.layout.is_unsized());

if place.layout.is_zst() {
return OperandRef::new_zst(self, place.layout);
return OperandRef::zero_sized(place.layout);
}

fn scalar_load_metadata<'a, 'gcc, 'tcx>(bx: &mut Builder<'a, 'gcc, 'tcx>, load: RValue<'gcc>, scalar: &abi::Scalar) {
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_codegen_gcc/src/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,7 @@ impl<'tcx> LayoutGccExt<'tcx> for TyAndLayout<'tcx> {
fn is_gcc_immediate(&self) -> bool {
match self.abi {
Abi::Scalar(_) | Abi::Vector { .. } => true,
Abi::ScalarPair(..) => false,
Abi::Uninhabited | Abi::Aggregate { .. } => self.is_zst(),
Abi::ScalarPair(..) | Abi::Uninhabited | Abi::Aggregate { .. } => false,
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
assert_eq!(place.llextra.is_some(), place.layout.is_unsized());

if place.layout.is_zst() {
return OperandRef::new_zst(self, place.layout);
return OperandRef::zero_sized(place.layout);
}

#[instrument(level = "trace", skip(bx))]
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_codegen_llvm/src/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,7 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> {
fn is_llvm_immediate(&self) -> bool {
match self.abi {
Abi::Scalar(_) | Abi::Vector { .. } => true,
Abi::ScalarPair(..) => false,
Abi::Uninhabited | Abi::Aggregate { .. } => self.is_zst(),
Abi::ScalarPair(..) | Abi::Uninhabited | Abi::Aggregate { .. } => false,
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ pub fn coerce_unsized_into<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let (base, info) = match bx.load_operand(src).val {
OperandValue::Pair(base, info) => unsize_ptr(bx, base, src_ty, dst_ty, Some(info)),
OperandValue::Immediate(base) => unsize_ptr(bx, base, src_ty, dst_ty, None),
OperandValue::Ref(..) => bug!(),
OperandValue::Ref(..) | OperandValue::ZeroSized => bug!(),
};
OperandValue::Pair(base, info).store(bx, dst);
}
Expand Down
13 changes: 12 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::operand::OperandRef;
use super::operand::OperandValue::{Immediate, Pair, Ref};
use super::operand::OperandValue::{Immediate, Pair, Ref, ZeroSized};
use super::place::PlaceRef;
use super::{CachedLlbb, FunctionCx, LocalRef};

Expand Down Expand Up @@ -427,6 +427,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
assert_eq!(align, op.layout.align.abi, "return place is unaligned!");
llval
}
ZeroSized => bug!("ZST return value shouldn't be in PassMode::Cast"),
};
let ty = bx.cast_backend_type(cast_ty);
let addr = bx.pointercast(llslot, bx.type_ptr_to(ty));
Expand Down Expand Up @@ -1386,6 +1387,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
(llval, align, true)
}
}
ZeroSized => match arg.mode {
PassMode::Indirect { .. } => {
// Though `extern "Rust"` doesn't pass ZSTs, some ABIs pass
// a pointer for `repr(C)` structs even when empty, so get
// one from an `alloca` (which can be left uninitialized).
let scratch = PlaceRef::alloca(bx, arg.layout);
(scratch.llval, scratch.align, true)
}
_ => bug!("ZST {op:?} wasn't ignored, but was passed with abi {arg:?}"),
},
};

if by_ref && !arg.is_indirect() {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx.set_var_name(a, &(name.clone() + ".0"));
bx.set_var_name(b, &(name.clone() + ".1"));
}
OperandValue::ZeroSized => {
// These never have a value to talk about
}
},
LocalRef::PendingOperand => {}
}
Expand Down
13 changes: 5 additions & 8 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,13 @@ enum LocalRef<'tcx, V> {
PendingOperand,
}

impl<'a, 'tcx, V: CodegenObject> LocalRef<'tcx, V> {
fn new_operand<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
bx: &mut Bx,
layout: TyAndLayout<'tcx>,
) -> LocalRef<'tcx, V> {
impl<'tcx, V: CodegenObject> LocalRef<'tcx, V> {
fn new_operand(layout: TyAndLayout<'tcx>) -> LocalRef<'tcx, V> {
if layout.is_zst() {
// Zero-size temporaries aren't always initialized, which
// doesn't matter because they don't contain data, but
// we need something in the operand.
LocalRef::Operand(OperandRef::new_zst(bx, layout))
LocalRef::Operand(OperandRef::zero_sized(layout))
} else {
LocalRef::PendingOperand
}
Expand Down Expand Up @@ -249,7 +246,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
}
} else {
debug!("alloc: {:?} -> operand", local);
LocalRef::new_operand(&mut start_bx, layout)
LocalRef::new_operand(layout)
}
};

Expand Down Expand Up @@ -355,7 +352,7 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let local = |op| LocalRef::Operand(op);
match arg.mode {
PassMode::Ignore => {
return local(OperandRef::new_zst(bx, arg.layout));
return local(OperandRef::zero_sized(arg.layout));
}
PassMode::Direct(_) => {
let llarg = bx.get_param(llarg_idx);
Expand Down
47 changes: 25 additions & 22 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ pub enum OperandValue<V> {
/// as returned by [`LayoutTypeMethods::scalar_pair_element_backend_type`]
/// with `immediate: true`.
Pair(V, V),
/// A value taking no bytes, and which therefore needs no LLVM value at all.
///
/// If you ever need a `V` to pass to something, get a fresh poison value
/// from [`ConstMethods::const_poison`].
///
/// An `OperandValue` *must* be this variant for any type for which
/// `is_zst` on its `Layout` returns `true`.
ZeroSized,
}

/// An `OperandRef` is an "SSA" reference to a Rust value, along with
Expand All @@ -71,15 +79,9 @@ impl<V: CodegenObject> fmt::Debug for OperandRef<'_, V> {
}

impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
pub fn new_zst<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
bx: &mut Bx,
layout: TyAndLayout<'tcx>,
) -> OperandRef<'tcx, V> {
pub fn zero_sized(layout: TyAndLayout<'tcx>) -> OperandRef<'tcx, V> {
assert!(layout.is_zst());
OperandRef {
val: OperandValue::Immediate(bx.const_poison(bx.immediate_backend_type(layout))),
layout,
}
OperandRef { val: OperandValue::ZeroSized, layout }
}

pub fn from_const<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
Expand All @@ -97,7 +99,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
let llval = bx.scalar_to_backend(x, scalar, bx.immediate_backend_type(layout));
OperandValue::Immediate(llval)
}
ConstValue::ZeroSized => return OperandRef::new_zst(bx, layout),
ConstValue::ZeroSized => return OperandRef::zero_sized(layout),
ConstValue::Slice { data, start, end } => {
let Abi::ScalarPair(a_scalar, _) = layout.abi else {
bug!("from_const: invalid ScalarPair layout: {:#?}", layout);
Expand Down Expand Up @@ -147,6 +149,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
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);
PlaceRef { llval: llptr, llextra, layout, align: layout.align.abi }
Expand Down Expand Up @@ -204,9 +207,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {

let mut val = match (self.val, self.layout.abi) {
// If the field is ZST, it has no data.
_ if field.is_zst() => {
return OperandRef::new_zst(bx, field);
}
_ if field.is_zst() => OperandValue::ZeroSized,

// Newtype of a scalar, scalar pair or vector.
(OperandValue::Immediate(_) | OperandValue::Pair(..), _)
Expand Down Expand Up @@ -237,6 +238,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
};

match (&mut val, field.abi) {
(OperandValue::ZeroSized, _) => {}
(
OperandValue::Immediate(llval),
Abi::Scalar(_) | Abi::ScalarPair(..) | Abi::Vector { .. },
Expand Down Expand Up @@ -290,16 +292,18 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
impl<'a, 'tcx, V: CodegenObject> OperandValue<V> {
/// Returns an `OperandValue` that's generally UB to use in any way.
///
/// Depending on the `layout`, returns an `Immediate` or `Pair` containing
/// poison value(s), or a `Ref` containing a poison pointer.
/// Depending on the `layout`, returns `ZeroSized` for ZSTs, an `Immediate` or
/// `Pair` containing poison value(s), or a `Ref` containing a poison pointer.
///
/// Supports sized types only.
pub fn poison<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
bx: &mut Bx,
layout: TyAndLayout<'tcx>,
) -> OperandValue<V> {
assert!(layout.is_sized());
if bx.cx().is_backend_immediate(layout) {
if layout.is_zst() {
OperandValue::ZeroSized
} else if bx.cx().is_backend_immediate(layout) {
let ibty = bx.cx().immediate_backend_type(layout);
OperandValue::Immediate(bx.const_poison(ibty))
} else if bx.cx().is_backend_scalar_pair(layout) {
Expand Down Expand Up @@ -352,12 +356,11 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> {
flags: MemFlags,
) {
debug!("OperandRef::store: operand={:?}, dest={:?}", self, dest);
// Avoid generating stores of zero-sized values, because the only way to have a zero-sized
// value is through `undef`, and store itself is useless.
if dest.layout.is_zst() {
return;
}
match self {
OperandValue::ZeroSized => {
// Avoid generating stores of zero-sized values, because the only way to have a zero-sized
// value is through `undef`/`poison`, and the store itself is useless.
}
OperandValue::Ref(r, None, source_align) => {
if flags.contains(MemFlags::NONTEMPORAL) {
// HACK(nox): This is inefficient but there is no nontemporal memcpy.
Expand Down Expand Up @@ -458,7 +461,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// checks in `codegen_consume` and `extract_field`.
let elem = o.layout.field(bx.cx(), 0);
if elem.is_zst() {
o = OperandRef::new_zst(bx, elem);
o = OperandRef::zero_sized(elem);
} else {
return None;
}
Expand Down Expand Up @@ -492,7 +495,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

// ZSTs don't require any actual memory access.
if layout.is_zst() {
return OperandRef::new_zst(bx, layout);
return OperandRef::zero_sized(layout);
}

if let Some(o) = self.maybe_codegen_consume_direct(bx, place_ref) {
Expand Down
Loading

0 comments on commit 9495095

Please sign in to comment.