-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
codegen: skip stores for entirely-uninit constant aggregate fields #157797
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ use rustc_target::callconv::{ArgAbi, ArgAttributes, CastTarget, FnAbi, PassMode} | |
| use tracing::{debug, info}; | ||
|
|
||
| use super::operand::OperandRef; | ||
| use super::operand::OperandValue::{self, Immediate, Pair, Ref, ZeroSized}; | ||
| use super::operand::OperandValue::{self, Immediate, Pair, Ref, Uninit, ZeroSized}; | ||
| use super::place::{PlaceRef, PlaceValue}; | ||
| use super::{CachedLlbb, FunctionCx, LocalRef}; | ||
| use crate::base::{self, is_call_from_compiler_builtins_to_upstream_monomorphization}; | ||
|
|
@@ -599,6 +599,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { | |
| place_val.llval | ||
| } | ||
| ZeroSized => bug!("ZST return value shouldn't be in PassMode::Cast"), | ||
| OperandValue::Uninit => { | ||
| bug!("uninit return value shouldn't be in PassMode::Cast") | ||
| } | ||
| }; | ||
|
|
||
| if self.fn_abi.conv == CanonAbi::Arm(ArmCall::CCmseNonSecureEntry) { | ||
|
|
@@ -1788,7 +1791,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { | |
|
|
||
| // Force by-ref if we have to load through a cast pointer. | ||
| let (mut llval, align, by_ref) = match op.val { | ||
| Immediate(_) | Pair(..) => match arg.mode { | ||
| Immediate(_) | Pair(..) | Uninit => match arg.mode { | ||
| PassMode::Indirect { attrs, .. } => { | ||
| // Indirect argument may have higher alignment requirements than the type's | ||
| // alignment. This can happen, e.g. when passing types with <4 byte alignment | ||
|
|
@@ -1808,7 +1811,14 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { | |
| op.store_with_annotation(bx, scratch); | ||
| (scratch.val.llval, scratch.val.align, true) | ||
| } | ||
| PassMode::Direct(_) => (op.immediate(), arg.layout.align.abi, false), | ||
| PassMode::Direct(_) => { | ||
| if let Uninit = op.val { | ||
| let ibty = bx.cx().immediate_backend_type(arg.layout); | ||
| (bx.cx().const_undef(ibty), arg.layout.align.abi, false) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are some sites const_undef and some const_poison?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intent is to use const_undef for cases that aren't UB, and const_poison for cases that are. |
||
| } else { | ||
| (op.immediate(), arg.layout.align.abi, false) | ||
| } | ||
| } | ||
| PassMode::Ignore | PassMode::Pair(..) => unreachable!("handled above"), | ||
| }, | ||
| Ref(op_place_val) => match arg.mode { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,12 @@ pub enum OperandValue<V> { | |
| /// `is_zst` on its `Layout` returns `true`. Note however that | ||
| /// these values can still require alignment. | ||
| ZeroSized, | ||
| /// A value for which all bytes are entirely uninitialized. | ||
| /// | ||
| /// Storing this value is a no-op; it propagates through field extraction. | ||
| /// Used to avoid emitting memcpys from uninit globals (which LLVM may | ||
| /// otherwise materialize as zero-fills) for `const <uninit>` operands. | ||
| Uninit, | ||
| } | ||
|
|
||
| impl<V: CodegenObject> OperandValue<V> { | ||
|
|
@@ -95,7 +101,7 @@ impl<V: CodegenObject> OperandValue<V> { | |
| match self { | ||
| OperandValue::Immediate(llptr) => Some((llptr, None)), | ||
| OperandValue::Pair(llptr, llextra) => Some((llptr, Some(llextra))), | ||
| OperandValue::Ref(_) | OperandValue::ZeroSized => None, | ||
| OperandValue::Ref(_) | OperandValue::ZeroSized | OperandValue::Uninit => None, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -123,6 +129,7 @@ impl<V: CodegenObject> OperandValue<V> { | |
| #[must_use] | ||
| pub(crate) fn is_expected_variant_for_type<'tcx>(&self, ty: TyAndLayout<'tcx>) -> bool { | ||
| match (self, ty.backend_repr) { | ||
| (OperandValue::Uninit, _) => true, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unfortunate because it means that there's no longer a fixed variant if you know the layout. It risks every https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/mir/operand/struct.OperandRef.html#method.immediate being a potential ICE now when it wasn't before :/ (Or other similar problems -- just fixing that method wouldn't be enough.) |
||
| (OperandValue::ZeroSized, BackendRepr::Memory { .. }) => ty.is_zst(), | ||
| (OperandValue::Ref(_), BackendRepr::Memory { .. }) => !ty.is_zst(), | ||
| ( | ||
|
|
@@ -397,7 +404,9 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { | |
| ); | ||
| } | ||
|
|
||
| let val = if field.is_zst() { | ||
| let val = if let OperandValue::Uninit = self.val { | ||
| OperandValue::Uninit | ||
| } else if field.is_zst() { | ||
| OperandValue::ZeroSized | ||
| } else if field.size == self.layout.size { | ||
| assert_eq!(offset.bytes(), 0); | ||
|
|
@@ -496,6 +505,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { | |
| // Read the tag/niche-encoded discriminant from memory. | ||
| let tag_op = match self.val { | ||
| OperandValue::ZeroSized => bug!(), | ||
| OperandValue::Uninit => return bx.cx().const_poison(cast_to), | ||
| OperandValue::Immediate(_) | OperandValue::Pair(_, _) => { | ||
| self.extract_field(fx, bx, tag_field.as_usize()) | ||
| } | ||
|
|
@@ -778,12 +788,15 @@ impl<'a, 'tcx, V: CodegenObject> OperandRefBuilder<'tcx, V> { | |
| field: FieldIdx, | ||
| field_operand: OperandRef<'tcx, V>, | ||
| ) { | ||
| if let OperandValue::ZeroSized = field_operand.val { | ||
| if matches!(field_operand.val, OperandValue::ZeroSized | OperandValue::Uninit) { | ||
| // A ZST never adds any state, so just ignore it. | ||
| // This special-casing is worth it because of things like | ||
| // `Result<!, !>` where `Ok(never)` is legal to write, | ||
| // but the type shows as FieldShape::Primitive so we can't | ||
| // actually look at the layout for the field being set. | ||
| // | ||
| // Likewise, an uninit field does not contribute any value; | ||
| // the builder's unset slots will produce `const_undef` in `build()`. | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1019,6 +1032,10 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> { | |
| // 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::Uninit => { | ||
| // Storing an entirely uninit value is a no-op: the destination is left | ||
| // uninitialized, which is valid since the value itself is uninit. | ||
| } | ||
| OperandValue::Ref(val) => { | ||
| assert!(dest.layout.is_sized(), "cannot directly store unsized values"); | ||
| if val.llextra.is_some() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // Regression test for https://github.com/rust-lang/rust/issues/157743 | ||
| // | ||
| // At opt-level >= 1, MIR GVN inlines MaybeUninit::uninit() and propagates the | ||
| // result as `const <uninit>` in aggregate constructions. Without the fix, this | ||
| // caused codegen to emit a memcpy from an `[N x i8] undef` global constant for | ||
| // the uninit field, which LLVM would materialize as zero-initialization. | ||
| // | ||
| // The fix skips emitting any IR for entirely-uninit constant aggregate fields. | ||
|
|
||
| //@ compile-flags: -C no-prepopulate-passes -C opt-level=2 | ||
|
|
||
| #![crate_type = "lib"] | ||
|
|
||
| use std::mem::MaybeUninit; | ||
|
|
||
| pub struct Inner { | ||
| cap: usize, | ||
| data: MaybeUninit<[u64; 2]>, | ||
| } | ||
|
|
||
| // CHECK-LABEL: @make_inner | ||
| // The non-uninit field must be stored. | ||
| // CHECK: store i{{(32|64)}} | ||
| // The entirely-uninit `data` field must not cause a memcpy from an undef global. | ||
| // CHECK-NOT: call void @llvm.memcpy | ||
| // CHECK: ret void | ||
| #[no_mangle] | ||
| pub fn make_inner(cap: usize) -> Inner { | ||
| Inner { cap, data: MaybeUninit::uninit() } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping uninit arguments looks wrong (similar for gcc implementation):