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

Lower to a memset(undef) when Rvalue::Repeat repeats uninit #138634

Merged
merged 1 commit into from
Mar 25, 2025
Merged
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
21 changes: 19 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
@@ -86,13 +86,30 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

mir::Rvalue::Repeat(ref elem, count) => {
let cg_elem = self.codegen_operand(bx, elem);

// Do not generate the loop for zero-sized elements or empty arrays.
if dest.layout.is_zst() {
Copy link
Member

Choose a reason for hiding this comment

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

pondering: with this change we'll skip evaluating the operand to repeat if the target is zero-sized.

I think that's ok, though? If it's a Move or Copy at worst it's not moveing the thing, but moving from something doesn't actually do anything right now anyway. And if it's a Constant something else is supposed to have evaluated it before we get here, right? So we don't need to worry about [const { panic!() }; 10] somehow getting skipped for a ZST element type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the whole mentioned items system is designed to guarantee that this isn't a problem:

//! One important role of collection is to evaluate all constants that are used by all the items
//! which are being collected. Codegen can then rely on only encountering constants that evaluate
//! successfully, and if a constant fails to evaluate, the collector has much better context to be
//! able to show where this constant comes up.

return;
}

// When the element is a const with all bytes uninit, emit a single memset that
// writes undef to the entire destination.
if let mir::Operand::Constant(const_op) = elem {
let val = self.eval_mir_constant(const_op);
if val.all_bytes_uninit(self.cx.tcx()) {
let size = bx.const_usize(dest.layout.size.bytes());
bx.memset(
dest.val.llval,
bx.const_undef(bx.type_i8()),
size,
dest.val.align,
MemFlags::empty(),
);
Comment on lines +100 to +106
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, llvm accepts storing undef even for things like array types https://llvm.godbolt.org/z/jYjaEYsTh -- even though using those types like that is wrong in most places -- so I don't actually know what's better here.

Eh, leave it as this is probably fine, since it's to llvm.memset (not C's memset) so the undef` to it probably isn't UB.

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 figured we can memcpy something that's undef, so surely we can memset with an uninit u8.

return;
}
}

let cg_elem = self.codegen_operand(bx, elem);

let try_init_all_same = |bx: &mut Bx, v| {
let start = dest.val.llval;
let size = bx.const_usize(dest.layout.size.bytes());
28 changes: 26 additions & 2 deletions compiler/rustc_middle/src/mir/consts.rs
Original file line number Diff line number Diff line change
@@ -9,7 +9,9 @@ use rustc_span::{DUMMY_SP, Span, Symbol};
use rustc_type_ir::TypeVisitableExt;

use super::interpret::ReportedErrorInfo;
use crate::mir::interpret::{AllocId, ConstAllocation, ErrorHandled, Scalar, alloc_range};
use crate::mir::interpret::{
AllocId, AllocRange, ConstAllocation, ErrorHandled, GlobalAlloc, Scalar, alloc_range,
};
use crate::mir::{Promoted, pretty_print_const_value};
use crate::ty::print::{pretty_print_const, with_no_trimmed_paths};
use crate::ty::{self, ConstKind, GenericArgsRef, ScalarInt, Ty, TyCtxt};
@@ -192,9 +194,31 @@ impl<'tcx> ConstValue<'tcx> {
.unwrap_memory()
.inner()
.provenance()
.range_empty(super::AllocRange::from(offset..offset + size), &tcx),
.range_empty(AllocRange::from(offset..offset + size), &tcx),
}
}

/// Check if a constant only contains uninitialized bytes.
pub fn all_bytes_uninit(&self, tcx: TyCtxt<'tcx>) -> bool {
let ConstValue::Indirect { alloc_id, .. } = self else {
return false;
};
let alloc = tcx.global_alloc(*alloc_id);
let GlobalAlloc::Memory(alloc) = alloc else {
return false;
};
let init_mask = alloc.0.init_mask();
let init_range = init_mask.is_range_initialized(AllocRange {
start: Size::ZERO,
size: Size::from_bytes(alloc.0.len()),
});
if let Err(range) = init_range {
if range.size == alloc.0.size() {
return true;
}
}
false
}
}

///////////////////////////////////////////////////////////////////////////
21 changes: 21 additions & 0 deletions tests/codegen/uninit-repeat-in-aggregate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//@ compile-flags: -Copt-level=3

#![crate_type = "lib"]

use std::mem::MaybeUninit;

// We need to make sure len is at offset 0, otherwise codegen needs an extra instruction
#[repr(C)]
pub struct SmallVec<T> {
pub len: u64,
pub arr: [MaybeUninit<T>; 24],
}

// CHECK-LABEL: @uninit_arr_via_const
#[no_mangle]
pub fn uninit_arr_via_const() -> SmallVec<String> {
// CHECK-NEXT: start:
// CHECK-NEXT: store i64 0,
// CHECK-NEXT: ret
SmallVec { len: 0, arr: [const { MaybeUninit::uninit() }; 24] }
}