-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Fix MaybeUninit codegen using GVN #147827
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 |
|---|---|---|
|
|
@@ -24,6 +24,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { | |
| ) { | ||
| match *rvalue { | ||
| mir::Rvalue::Use(ref operand) => { | ||
| if let mir::Operand::Constant(const_op) = operand { | ||
| let val = self.eval_mir_constant(&const_op); | ||
| if val.all_bytes_uninit(self.cx.tcx()) { | ||
| return; | ||
| } | ||
| } | ||
| let cg_operand = self.codegen_operand(bx, operand); | ||
|
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. Actually, would it make sense for Hmm, no, that's non-trivial isn't it, because there's nothing good to make |
||
| // Crucially, we do *not* use `OperandValue::Ref` for types with | ||
| // `BackendRepr::Scalar | BackendRepr::ScalarPair`. This ensures we match the MIR | ||
|
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. annoyingly it won't let me comment on the actual line, but I think the FIXME on line 43 below might be updatable now? |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1872,6 +1872,13 @@ fn pretty_print_const_value_tcx<'tcx>( | |
| return Ok(()); | ||
| } | ||
|
|
||
| // Printing [MaybeUninit<u8>::uninit(); N] or any other aggregate where all fields are uninit | ||
| // becomes very verbose. This special case makes the dump terse and clear. | ||
|
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. 👍 |
||
| if ct.all_bytes_uninit(tcx) { | ||
| fmt.write_str("<uninit>")?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let u8_type = tcx.types.u8; | ||
| match (ct, ty.kind()) { | ||
| // Byte/string slices, printed as (byte) string literals. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // This is a regression test for https://github.com/rust-lang/rust/issues/139355 | ||
|
|
||
| //@ compile-flags: -Copt-level=3 | ||
|
|
||
| #![crate_type = "lib"] | ||
|
|
||
| use std::mem::MaybeUninit; | ||
|
|
||
| #[no_mangle] | ||
| pub fn create_uninit_array() -> [[MaybeUninit<u8>; 4]; 200] { | ||
| // CHECK-LABEL: create_uninit_array | ||
| // CHECK-NEXT: start: | ||
| // CHECK-NEXT: ret void | ||
| [[MaybeUninit::<u8>::uninit(); 4]; 200] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| //@ compile-flags: -O | ||
|
|
||
| use std::mem::MaybeUninit; | ||
|
|
||
| // EMIT_MIR maybe_uninit.u8_array.GVN.diff | ||
| pub fn u8_array() -> [MaybeUninit<u8>; 8] { | ||
| // CHECK: fn u8_array( | ||
| // CHECK: _0 = const <uninit>; | ||
| [MaybeUninit::uninit(); 8] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| - // MIR for `u8_array` before GVN | ||
| + // MIR for `u8_array` after GVN | ||
|
|
||
| fn u8_array() -> [MaybeUninit<u8>; 8] { | ||
| let mut _0: [std::mem::MaybeUninit<u8>; 8]; | ||
| let mut _1: std::mem::MaybeUninit<u8>; | ||
| scope 1 (inlined MaybeUninit::<u8>::uninit) { | ||
| } | ||
|
|
||
| bb0: { | ||
| StorageLive(_1); | ||
| - _1 = MaybeUninit::<u8> { uninit: const () }; | ||
| - _0 = [move _1; 8]; | ||
| + _1 = const <uninit>; | ||
| + _0 = const <uninit>; | ||
| StorageDead(_1); | ||
| return; | ||
| } | ||
| + } | ||
| + | ||
| + ALLOC0 (size: 8, align: 1) { | ||
| + __ __ __ __ __ __ __ __ │ ░░░░░░░░ | ||
| + } | ||
| + | ||
| + ALLOC1 (size: 1, align: 1) { | ||
| + __ │ ░ | ||
| } | ||
|
|
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.
pondering: I wonder if there's any places that are intentionally writing uninit somewhere to tell LLVM that the value doesn't matter. It might be interesting to (outside of debug) write
undefto the place for the situations where LLVM doesn't do the bad behaviour of copying a constant for that (such as*p = undef;for*pbeingScalarorScalarPair).But I don't think that would need to be in this PR.