Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "store ScalarPair via memset when one side is undef and the other side can be memset" #137894

Merged
merged 2 commits into from
Mar 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions compiler/rustc_codegen_gcc/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,6 @@ impl<'gcc, 'tcx> ConstCodegenMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
if type_is_pointer(typ) { self.context.new_null(typ) } else { self.const_int(typ, 0) }
}

fn is_undef(&self, _val: RValue<'gcc>) -> bool {
// FIXME: actually check for undef
false
}

fn const_undef(&self, typ: Type<'gcc>) -> RValue<'gcc> {
let local = self.current_func.borrow().expect("func").new_local(None, typ, "undefined");
if typ.is_struct().is_some() {
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_codegen_llvm/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,6 @@ impl<'ll, 'tcx> ConstCodegenMethods<'tcx> for CodegenCx<'ll, 'tcx> {
unsafe { llvm::LLVMGetUndef(t) }
}

fn is_undef(&self, v: &'ll Value) -> bool {
unsafe { llvm::LLVMIsUndef(v) == True }
}

fn const_poison(&self, t: &'ll Type) -> &'ll Value {
unsafe { llvm::LLVMGetPoison(t) }
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,6 @@ unsafe extern "C" {
pub(crate) fn LLVMMetadataTypeInContext(C: &Context) -> &Type;

// Operations on all values
pub(crate) fn LLVMIsUndef(Val: &Value) -> Bool;
pub(crate) fn LLVMTypeOf(Val: &Value) -> &Type;
pub(crate) fn LLVMGetValueName2(Val: &Value, Length: *mut size_t) -> *const c_char;
pub(crate) fn LLVMSetValueName2(Val: &Value, Name: *const c_char, NameLen: size_t);
Expand Down
59 changes: 22 additions & 37 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,30 +203,14 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
let alloc_align = alloc.inner().align;
assert!(alloc_align >= layout.align.abi);

// Returns `None` when the value is partially undefined or any byte of it has provenance.
// Otherwise returns the value or (if the entire value is undef) returns an undef.
let read_scalar = |start, size, s: abi::Scalar, ty| {
let range = alloc_range(start, size);
match alloc.0.read_scalar(
bx,
range,
alloc_range(start, size),
/*read_provenance*/ matches!(s.primitive(), abi::Primitive::Pointer(_)),
) {
Ok(val) => Some(bx.scalar_to_backend(val, s, ty)),
Err(_) => {
// We may have failed due to partial provenance or unexpected provenance,
// continue down the normal code path if so.
if alloc.0.provenance().range_empty(range, &bx.tcx())
// Since `read_scalar` failed, but there were no relocations involved, the
// bytes must be partially or fully uninitialized. Thus we can now unwrap the
// information about the range of uninit bytes and check if it's the full range.
&& alloc.0.init_mask().is_range_initialized(range).unwrap_err() == range
{
Some(bx.const_undef(ty))
} else {
None
}
}
Ok(val) => bx.scalar_to_backend(val, s, ty),
Err(_) => bx.const_poison(ty),
}
};

Expand All @@ -237,14 +221,16 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
// check that walks over the type of `mplace` to make sure it is truly correct to treat this
// like a `Scalar` (or `ScalarPair`).
match layout.backend_repr {
BackendRepr::Scalar(s) => {
BackendRepr::Scalar(s @ abi::Scalar::Initialized { .. }) => {
let size = s.size(bx);
assert_eq!(size, layout.size, "abi::Scalar size does not match layout size");
if let Some(val) = read_scalar(offset, size, s, bx.immediate_backend_type(layout)) {
return OperandRef { val: OperandValue::Immediate(val), layout };
}
let val = read_scalar(offset, size, s, bx.immediate_backend_type(layout));
OperandRef { val: OperandValue::Immediate(val), layout }
}
BackendRepr::ScalarPair(a, b) => {
BackendRepr::ScalarPair(
a @ abi::Scalar::Initialized { .. },
b @ abi::Scalar::Initialized { .. },
) => {
let (a_size, b_size) = (a.size(bx), b.size(bx));
let b_offset = (offset + a_size).align_to(b.align(bx).abi);
assert!(b_offset.bytes() > 0);
Expand All @@ -260,21 +246,20 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
b,
bx.scalar_pair_element_backend_type(layout, 1, true),
);
if let (Some(a_val), Some(b_val)) = (a_val, b_val) {
return OperandRef { val: OperandValue::Pair(a_val, b_val), layout };
}
OperandRef { val: OperandValue::Pair(a_val, b_val), layout }
}
_ if layout.is_zst() => OperandRef::zero_sized(layout),
_ => {
// Neither a scalar nor scalar pair. Load from a place
// FIXME: should we cache `const_data_from_alloc` to avoid repeating this for the
// same `ConstAllocation`?
let init = bx.const_data_from_alloc(alloc);
let base_addr = bx.static_addr_of(init, alloc_align, None);

let llval = bx.const_ptr_byte_offset(base_addr, offset);
bx.load_operand(PlaceRef::new_sized(llval, layout))
}
_ if layout.is_zst() => return OperandRef::zero_sized(layout),
_ => {}
}
// Neither a scalar nor scalar pair. Load from a place
// FIXME: should we cache `const_data_from_alloc` to avoid repeating this for the
// same `ConstAllocation`?
let init = bx.const_data_from_alloc(alloc);
let base_addr = bx.static_addr_of(init, alloc_align, None);

let llval = bx.const_ptr_byte_offset(base_addr, offset);
bx.load_operand(PlaceRef::new_sized(llval, layout))
}

/// Asserts that this operand refers to a scalar and returns
Expand Down
26 changes: 2 additions & 24 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
use rustc_middle::{bug, mir, span_bug};
use rustc_session::config::OptLevel;
use rustc_span::{DUMMY_SP, Span};
use tracing::{debug, instrument, trace};
use tracing::{debug, instrument};

use super::operand::{OperandRef, OperandValue};
use super::place::PlaceRef;
Expand Down Expand Up @@ -93,8 +93,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
return;
}

// If `v` is an integer constant whose value is just a single byte repeated N times,
// emit a `memset` filling the entire `dest` with that byte.
let try_init_all_same = |bx: &mut Bx, v| {
let start = dest.val.llval;
let size = bx.const_usize(dest.layout.size.bytes());
Expand All @@ -119,33 +117,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
false
};

trace!(?cg_elem.val);
match cg_elem.val {
OperandValue::Immediate(v) => {
if try_init_all_same(bx, v) {
return;
}
}
OperandValue::Pair(a, b) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, on line 104:

The try_init_all_same tries to access a value that should be size u128, but since scalar pairs can be twice the size of u128 it ends up indexing out of bounds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code will need to both check that the initialized half of the scalarpair is the same value, then emit another memset for the uninitialized part, and be a bit more careful to use the correct size of the value being passed into the function.

let a_is_undef = bx.cx().is_undef(a);
match (a_is_undef, bx.cx().is_undef(b)) {
// Can happen for uninit unions
(true, true) => {
// FIXME: can we produce better output here?
}
(false, true) | (true, false) => {
let val = if a_is_undef { b } else { a };
if try_init_all_same(bx, val) {
return;
}
}
(false, false) => {
// FIXME: if both are the same value, use try_init_all_same
}
}
}
OperandValue::ZeroSized => unreachable!("checked above"),
OperandValue::Ref(..) => {}
_ => (),
}

let count = self
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_codegen_ssa/src/traits/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ pub trait ConstCodegenMethods<'tcx>: BackendTypes {
/// Generate an uninitialized value (matching uninitialized memory in MIR).
/// Whether memory is initialized or not is tracked byte-for-byte.
fn const_undef(&self, t: Self::Type) -> Self::Value;
fn is_undef(&self, v: Self::Value) -> bool;
/// Generate a fake value. Poison always affects the entire value, even if just a single byte is
/// poison. This can only be used in codepaths that are already UB, i.e., UB-free Rust code
/// (including code that e.g. copies uninit memory with `MaybeUninit`) can never encounter a
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl AllocError {
}

/// The information that makes up a memory access: offset and size.
#[derive(Copy, Clone, PartialEq)]
#[derive(Copy, Clone)]
pub struct AllocRange {
pub start: Size,
pub size: Size,
Expand Down
51 changes: 8 additions & 43 deletions tests/codegen/slice-init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

#![crate_type = "lib"]

use std::mem::MaybeUninit;

// CHECK-LABEL: @zero_sized_elem
#[no_mangle]
pub fn zero_sized_elem() {
Expand Down Expand Up @@ -78,64 +76,31 @@ pub fn u16_init_one_bytes() -> [u16; N] {
[const { u16::from_be_bytes([1, 1]) }; N]
}

// FIXME: undef bytes can just be initialized with the same value as the
// defined bytes, if the defines bytes are all the same.
// CHECK-LABEL: @option_none_init
#[no_mangle]
pub fn option_none_init() -> [Option<u8>; N] {
// CHECK-NOT: select
// CHECK-NOT: br
// CHECK-NOT: switch
// CHECK-NOT: icmp
// CHECK: call void @llvm.memset.p0
[const { None }; N]
}

// If there is partial provenance or some bytes are initialized and some are not,
// we can't really do better than initialize bytes or groups of bytes together.
// CHECK-LABEL: @option_maybe_uninit_init
#[no_mangle]
pub fn option_maybe_uninit_init() -> [MaybeUninit<u16>; N] {
// CHECK-NOT: select
// CHECK: br label %repeat_loop_header{{.*}}
// CHECK-NOT: switch
// CHECK: icmp
// CHECK-NOT: call void @llvm.memset.p0
[const {
let mut val: MaybeUninit<u16> = MaybeUninit::uninit();
let ptr = val.as_mut_ptr() as *mut u8;
unsafe {
ptr.write(0);
}
val
}; N]
[None; N]
}

#[repr(packed)]
struct Packed {
start: u8,
ptr: &'static (),
rest: u16,
rest2: u8,
}
use std::mem::MaybeUninit;

// If there is partial provenance or some bytes are initialized and some are not,
// we can't really do better than initialize bytes or groups of bytes together.
// CHECK-LABEL: @option_maybe_uninit_provenance
// FIXME: This could be optimized into a memset.
// Regression test for <https://github.com/rust-lang/rust/issues/137892>.
#[no_mangle]
pub fn option_maybe_uninit_provenance() -> [MaybeUninit<Packed>; N] {
pub fn half_uninit() -> [(u128, MaybeUninit<u128>); N] {
// CHECK-NOT: select
// CHECK: br label %repeat_loop_header{{.*}}
// CHECK-NOT: switch
// CHECK: icmp
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied all this from above :clueless:

// CHECK-NOT: call void @llvm.memset.p0
[const {
let mut val: MaybeUninit<Packed> = MaybeUninit::uninit();
unsafe {
let ptr = &raw mut (*val.as_mut_ptr()).ptr;
static HAS_ADDR: () = ();
ptr.write_unaligned(&HAS_ADDR);
}
val
}; N]
[const { (0, MaybeUninit::uninit()) }; N]
}

// Use an opaque function to prevent rustc from removing useless drops.
Expand Down
Loading