From 6b96d17a5a7f7e9a32e9aa24c34d858bf4394571 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 14 May 2020 10:11:15 -0700 Subject: [PATCH 01/11] Dumb NRVO --- src/librustc_mir/transform/mod.rs | 2 + src/librustc_mir/transform/nrvo.rs | 219 +++++++++++++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 src/librustc_mir/transform/nrvo.rs diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 7ad5baac205..0551ed5a15d 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -28,6 +28,7 @@ pub mod generator; pub mod inline; pub mod instcombine; pub mod no_landing_pads; +pub mod nrvo; pub mod promote_consts; pub mod qualify_min_const_fn; pub mod remove_noop_landing_pads; @@ -324,6 +325,7 @@ fn run_optimization_passes<'tcx>( &remove_noop_landing_pads::RemoveNoopLandingPads, &simplify::SimplifyCfg::new("after-remove-noop-landing-pads"), &simplify::SimplifyCfg::new("final"), + &nrvo::RenameReturnPlace, &simplify::SimplifyLocals, ]; diff --git a/src/librustc_mir/transform/nrvo.rs b/src/librustc_mir/transform/nrvo.rs new file mode 100644 index 00000000000..c33e2eb88e1 --- /dev/null +++ b/src/librustc_mir/transform/nrvo.rs @@ -0,0 +1,219 @@ +use rustc_index::bit_set::HybridBitSet; +use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor}; +use rustc_middle::mir::{self, BasicBlock, Local, Location}; +use rustc_middle::ty::TyCtxt; + +use crate::transform::{MirPass, MirSource}; + +/// This pass looks for MIR that always copies the same local into the return place and eliminates +/// the copy by renaming all uses of that local to `_0`. +/// +/// This allows LLVM to perform an optimization similar to the named return value optimization +/// (NRVO) that is guaranteed in C++. This avoids a stack allocation and `memcpy` for the +/// relatively common pattern of allocating a buffer on the stack, mutating it, and returning it by +/// value like so: +/// +/// ```rust +/// fn foo(init: fn(&mut [u8; 1024])) -> [u8; 1024] { +/// let mut buf = [0; 1024]; +/// init(&mut buf); +/// buf +/// } +/// ``` +/// +/// For now, this pass is very simple and only capable of eliminating a single copy. +/// A more general version of copy propagation could yield even more benefits. +pub struct RenameReturnPlace; + +impl<'tcx> MirPass<'tcx> for RenameReturnPlace { + fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut mir::Body<'tcx>) { + if tcx.sess.opts.debugging_opts.mir_opt_level == 0 { + return; + } + + let returned_local = match local_eligible_for_nrvo(body) { + Some(l) => l, + None => { + debug!("`{:?}` was ineligible for NRVO", src.def_id()); + return; + } + }; + + debug!( + "`{:?}` was eligible for NRVO, making {:?} the return place", + src.def_id(), + returned_local + ); + + RenameToReturnPlace { tcx, to_rename: returned_local }.visit_body(body); + + // Clean up the `NOP`s we inserted for statements made useless by our renaming. + for block_data in body.basic_blocks_mut() { + block_data.statements.retain(|stmt| stmt.kind != mir::StatementKind::Nop); + } + + // Overwrite the debuginfo of `_0` with that of the renamed local. + let (renamed_decl, ret_decl) = + body.local_decls.pick2_mut(returned_local, mir::RETURN_PLACE); + debug_assert_eq!(ret_decl.ty, renamed_decl.ty); + ret_decl.clone_from(renamed_decl); + } +} + +/// MIR that is eligible for the NRVO must fulfill two conditions: +/// 1. The return place must not be read prior to the `Return` terminator. +/// 2. A simple assignment of a whole local to the return place (e.g., `_0 = _1`) must be the +/// only definition of the return place reaching the `Return` terminator. +/// +/// If the MIR fulfills both these conditions, this function returns the `Local` that is assigned +/// to the return place along all possible paths through the control-flow graph. +fn local_eligible_for_nrvo(body: &mut mir::Body<'_>) -> Option { + if IsReturnPlaceRead::run(body) { + return None; + } + + let mut copied_to_return_place = None; + for block in body.basic_blocks().indices() { + // Look for blocks with a `Return` terminator. + if !matches!(body[block].terminator().kind, mir::TerminatorKind::Return) { + continue; + } + + // Look for an assignment of a single local to the return place prior to the `Return`. + let returned_local = find_local_assigned_to_return_place(block, body)?; + match body.local_kind(returned_local) { + // FIXME: Can we do this for arguments as well? + mir::LocalKind::Arg => return None, + + mir::LocalKind::ReturnPointer => bug!("Return place was assigned to itself?"), + mir::LocalKind::Var | mir::LocalKind::Temp => {} + } + + // If multiple different locals are copied to the return place. We can't pick a + // single one to rename. + if copied_to_return_place.map_or(false, |old| old != returned_local) { + return None; + } + + copied_to_return_place = Some(returned_local); + } + + return copied_to_return_place; +} + +fn find_local_assigned_to_return_place( + start: BasicBlock, + body: &mut mir::Body<'_>, +) -> Option { + let mut block = start; + let mut seen = HybridBitSet::new_empty(body.basic_blocks().len()); + + // Iterate as long as `block` has exactly one predecessor that we have not yet visited. + while seen.insert(block) { + trace!("Looking for assignments to `_0` in {:?}", block); + + let local = body[block].statements.iter().rev().find_map(as_local_assigned_to_return_place); + if local.is_some() { + return local; + } + + match body.predecessors()[block].as_slice() { + &[pred] => block = pred, + _ => return None, + } + } + + return None; +} + +// If this statement is an assignment of an unprojected local to the return place, +// return that local. +fn as_local_assigned_to_return_place(stmt: &mir::Statement<'_>) -> Option { + if let mir::StatementKind::Assign(box (lhs, rhs)) = &stmt.kind { + if lhs.as_local() == Some(mir::RETURN_PLACE) { + if let mir::Rvalue::Use(mir::Operand::Copy(rhs) | mir::Operand::Move(rhs)) = rhs { + return rhs.as_local(); + } + } + } + + None +} + +struct RenameToReturnPlace<'tcx> { + to_rename: Local, + tcx: TyCtxt<'tcx>, +} + +/// Replaces all uses of `self.to_rename` with `_0`. +impl MutVisitor<'tcx> for RenameToReturnPlace<'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn visit_statement(&mut self, stmt: &mut mir::Statement<'tcx>, loc: Location) { + // Remove assignments of the local being replaced to the return place, since it is now the + // return place: + // _0 = _1 + if as_local_assigned_to_return_place(stmt) == Some(self.to_rename) { + stmt.kind = mir::StatementKind::Nop; + return; + } + + // Remove storage annotations for the local being replaced: + // StorageLive(_1) + if let mir::StatementKind::StorageLive(local) | mir::StatementKind::StorageDead(local) = + stmt.kind + { + if local == self.to_rename { + stmt.kind = mir::StatementKind::Nop; + return; + } + } + + self.super_statement(stmt, loc) + } + + fn visit_terminator(&mut self, terminator: &mut mir::Terminator<'tcx>, loc: Location) { + // Ignore the implicit "use" of the return place in a `Return` statement. + if let mir::TerminatorKind::Return = terminator.kind { + return; + } + + self.super_terminator(terminator, loc); + } + + fn visit_local(&mut self, l: &mut Local, _: PlaceContext, _: Location) { + assert_ne!(*l, mir::RETURN_PLACE); + if *l == self.to_rename { + *l = mir::RETURN_PLACE; + } + } +} + +struct IsReturnPlaceRead(bool); + +impl IsReturnPlaceRead { + fn run(body: &mir::Body<'_>) -> bool { + let mut vis = IsReturnPlaceRead(false); + vis.visit_body(body); + vis.0 + } +} + +impl Visitor<'tcx> for IsReturnPlaceRead { + fn visit_local(&mut self, &l: &Local, ctxt: PlaceContext, _: Location) { + if l == mir::RETURN_PLACE && ctxt.is_use() && !ctxt.is_place_assignment() { + self.0 = true; + } + } + + fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, loc: Location) { + // Ignore the implicit "use" of the return place in a `Return` statement. + if let mir::TerminatorKind::Return = terminator.kind { + return; + } + + self.super_terminator(terminator, loc); + } +} From 966df3e9266494771254db650cad478ca8c322e4 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 15 May 2020 14:53:20 -0700 Subject: [PATCH 02/11] Bless MIR tests that inline functions qualifying for NRVO --- .../rustc.a.Inline.after.mir | 6 +----- .../rustc.b.Inline.after.mir | 6 +----- .../rustc.d.Inline.after.mir | 6 +----- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir index d6c5220e28f..2eebf3f0ece 100644 --- a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir +++ b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.a.Inline.after.mir @@ -8,7 +8,6 @@ fn a(_1: &mut [T]) -> &mut [T] { let mut _4: &mut [T]; // in scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:6 scope 1 { debug self => _4; // in scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL - let mut _5: &mut [T]; // in scope 1 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 } bb0: { @@ -16,10 +15,7 @@ fn a(_1: &mut [T]) -> &mut [T] { StorageLive(_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 StorageLive(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:6 _4 = &mut (*_1); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:6 - StorageLive(_5); // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL - _5 = _4; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL - _3 = _5; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL - StorageDead(_5); // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL + _3 = _4; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL _2 = &mut (*_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 StorageDead(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:14: 3:15 _0 = &mut (*_2); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:3:5: 3:15 diff --git a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir index 2270abc288d..f9e1699c55d 100644 --- a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir +++ b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.b.Inline.after.mir @@ -9,7 +9,6 @@ fn b(_1: &mut std::boxed::Box) -> &mut T { scope 1 { debug self => _4; // in scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL let mut _5: &mut T; // in scope 1 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:15 - let mut _6: &mut T; // in scope 1 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:15 } bb0: { @@ -18,11 +17,8 @@ fn b(_1: &mut std::boxed::Box) -> &mut T { StorageLive(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:6 _4 = &mut (*_1); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:6 StorageLive(_5); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - StorageLive(_6); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - _6 = &mut (*(*_4)); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - _5 = _6; // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL + _5 = &mut (*(*_4)); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL _3 = _5; // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - StorageDead(_6); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL StorageDead(_5); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL _2 = &mut (*_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:5: 8:15 StorageDead(_4); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:8:14: 8:15 diff --git a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.d.Inline.after.mir b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.d.Inline.after.mir index a929cb1c2fb..08bd4784bde 100644 --- a/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.d.Inline.after.mir +++ b/src/test/mir-opt/inline/issue-58867-inline-as-ref-as-mut/rustc.d.Inline.after.mir @@ -7,17 +7,13 @@ fn d(_1: &std::boxed::Box) -> &T { let mut _3: &std::boxed::Box; // in scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:6 scope 1 { debug self => _3; // in scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - let _4: &T; // in scope 1 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:15 } bb0: { StorageLive(_2); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:15 StorageLive(_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:6 _3 = &(*_1); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:6 - StorageLive(_4); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - _4 = &(*(*_3)); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - _2 = _4; // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL - StorageDead(_4); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL + _2 = &(*(*_3)); // scope 1 at $SRC_DIR/liballoc/boxed.rs:LL:COL _0 = &(*_2); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:5: 18:15 StorageDead(_3); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:18:14: 18:15 StorageDead(_2); // scope 0 at $DIR/issue-58867-inline-as-ref-as-mut.rs:19:1: 19:2 From 2cba138f4a65a3772c34b240b2a3e64f276083ec Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 15 May 2020 14:57:42 -0700 Subject: [PATCH 03/11] Add simple NRVO test --- src/test/mir-opt/nrvo-simple.rs | 10 +++++ .../rustc.nrvo.RenameReturnPlace.diff | 41 +++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 src/test/mir-opt/nrvo-simple.rs create mode 100644 src/test/mir-opt/nrvo-simple/rustc.nrvo.RenameReturnPlace.diff diff --git a/src/test/mir-opt/nrvo-simple.rs b/src/test/mir-opt/nrvo-simple.rs new file mode 100644 index 00000000000..bf3a0efeada --- /dev/null +++ b/src/test/mir-opt/nrvo-simple.rs @@ -0,0 +1,10 @@ +// EMIT_MIR rustc.nrvo.RenameReturnPlace.diff +fn nrvo(init: fn(&mut [u8; 1024])) -> [u8; 1024] { + let mut buf = [0; 1024]; + init(&mut buf); + buf +} + +fn main() { + let _ = nrvo(|buf| { buf[4] = 4; }); +} diff --git a/src/test/mir-opt/nrvo-simple/rustc.nrvo.RenameReturnPlace.diff b/src/test/mir-opt/nrvo-simple/rustc.nrvo.RenameReturnPlace.diff new file mode 100644 index 00000000000..79d92897cb5 --- /dev/null +++ b/src/test/mir-opt/nrvo-simple/rustc.nrvo.RenameReturnPlace.diff @@ -0,0 +1,41 @@ +- // MIR for `nrvo` before RenameReturnPlace ++ // MIR for `nrvo` after RenameReturnPlace + + fn nrvo(_1: for<'r> fn(&'r mut [u8; 1024])) -> [u8; 1024] { + debug init => _1; // in scope 0 at $DIR/nrvo-simple.rs:2:9: 2:13 +- let mut _0: [u8; 1024]; // return place in scope 0 at $DIR/nrvo-simple.rs:2:39: 2:49 ++ let mut _0: [u8; 1024]; // return place in scope 0 at $DIR/nrvo-simple.rs:3:9: 3:16 + let mut _2: [u8; 1024]; // in scope 0 at $DIR/nrvo-simple.rs:3:9: 3:16 + let _3: (); // in scope 0 at $DIR/nrvo-simple.rs:4:5: 4:19 + let mut _4: for<'r> fn(&'r mut [u8; 1024]); // in scope 0 at $DIR/nrvo-simple.rs:4:5: 4:9 + let mut _5: &mut [u8; 1024]; // in scope 0 at $DIR/nrvo-simple.rs:4:10: 4:18 + let mut _6: &mut [u8; 1024]; // in scope 0 at $DIR/nrvo-simple.rs:4:10: 4:18 + scope 1 { +- debug buf => _2; // in scope 1 at $DIR/nrvo-simple.rs:3:9: 3:16 ++ debug buf => _0; // in scope 1 at $DIR/nrvo-simple.rs:3:9: 3:16 + } + + bb0: { +- StorageLive(_2); // scope 0 at $DIR/nrvo-simple.rs:3:9: 3:16 +- _2 = [const 0u8; 1024]; // scope 0 at $DIR/nrvo-simple.rs:3:19: 3:28 ++ _0 = [const 0u8; 1024]; // scope 0 at $DIR/nrvo-simple.rs:3:19: 3:28 + // ty::Const + // + ty: u8 + // + val: Value(Scalar(0x00)) + // mir::Constant + // + span: $DIR/nrvo-simple.rs:3:20: 3:21 + // + literal: Const { ty: u8, val: Value(Scalar(0x00)) } + StorageLive(_3); // scope 1 at $DIR/nrvo-simple.rs:4:5: 4:19 +- _6 = &mut _2; // scope 1 at $DIR/nrvo-simple.rs:4:10: 4:18 ++ _6 = &mut _0; // scope 1 at $DIR/nrvo-simple.rs:4:10: 4:18 + _3 = move _1(move _6) -> bb1; // scope 1 at $DIR/nrvo-simple.rs:4:5: 4:19 + } + + bb1: { + StorageDead(_3); // scope 1 at $DIR/nrvo-simple.rs:4:19: 4:20 +- _0 = _2; // scope 1 at $DIR/nrvo-simple.rs:5:5: 5:8 +- StorageDead(_2); // scope 0 at $DIR/nrvo-simple.rs:6:1: 6:2 + return; // scope 0 at $DIR/nrvo-simple.rs:6:2: 6:2 + } + } + From e369d7f4e7cb3dff060dedabb1356b68ff3422c8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 16 May 2020 11:59:30 -0700 Subject: [PATCH 04/11] Expand comment with possible improvements --- src/librustc_mir/transform/nrvo.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/nrvo.rs b/src/librustc_mir/transform/nrvo.rs index c33e2eb88e1..557ffe4a6d5 100644 --- a/src/librustc_mir/transform/nrvo.rs +++ b/src/librustc_mir/transform/nrvo.rs @@ -21,8 +21,12 @@ use crate::transform::{MirPass, MirSource}; /// } /// ``` /// -/// For now, this pass is very simple and only capable of eliminating a single copy. -/// A more general version of copy propagation could yield even more benefits. +/// For now, this pass is very simple and only capable of eliminating a single copy. A more general +/// version of copy propagation, such as the one based on non-overlapping live ranges in [#47954] and +/// [#71003], could yield even more benefits. +/// +/// [#47954]: https://github.com/rust-lang/rust/pull/47954 +/// [#71003]: https://github.com/rust-lang/rust/pull/71003 pub struct RenameReturnPlace; impl<'tcx> MirPass<'tcx> for RenameReturnPlace { From c38283d7a78d005edd36139879e979037786bd06 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 16 May 2020 12:15:25 -0700 Subject: [PATCH 05/11] Test that Miri can handle MIR with NRVO applied --- src/test/ui/consts/miri_unleashed/nrvo.rs | 23 ++++++++++++++++ src/test/ui/consts/miri_unleashed/nrvo.stderr | 27 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 src/test/ui/consts/miri_unleashed/nrvo.rs create mode 100644 src/test/ui/consts/miri_unleashed/nrvo.stderr diff --git a/src/test/ui/consts/miri_unleashed/nrvo.rs b/src/test/ui/consts/miri_unleashed/nrvo.rs new file mode 100644 index 00000000000..b7f11024805 --- /dev/null +++ b/src/test/ui/consts/miri_unleashed/nrvo.rs @@ -0,0 +1,23 @@ +// compile-flags: -Zunleash-the-miri-inside-of-you +// run-pass + +const fn init(buf: &mut [u8; 1024]) { + buf[33] = 3; + buf[444] = 4; +} + +const fn nrvo(init: fn(&mut [u8; 1024])) -> [u8; 1024] { + let mut buf = [0; 1024]; + init(&mut buf); + buf +} + +// When the NRVO is applied, the return place (`_0`) gets treated like a normal local. For example, +// its address may be taken and it may be written to indirectly. Ensure that MIRI can handle this. +const BUF: [u8; 1024] = nrvo(init); + +fn main() { + assert_eq!(BUF[33], 3); + assert_eq!(BUF[19], 0); + assert_eq!(BUF[444], 4); +} diff --git a/src/test/ui/consts/miri_unleashed/nrvo.stderr b/src/test/ui/consts/miri_unleashed/nrvo.stderr new file mode 100644 index 00000000000..d38f869181b --- /dev/null +++ b/src/test/ui/consts/miri_unleashed/nrvo.stderr @@ -0,0 +1,27 @@ +warning: skipping const checks + | +help: skipping check for `const_mut_refs` feature + --> $DIR/nrvo.rs:5:5 + | +LL | buf[33] = 3; + | ^^^^^^^^^^^ +help: skipping check for `const_mut_refs` feature + --> $DIR/nrvo.rs:6:5 + | +LL | buf[444] = 4; + | ^^^^^^^^^^^^ +help: skipping check for `const_mut_refs` feature + --> $DIR/nrvo.rs:11:10 + | +LL | init(&mut buf); + | ^^^^^^^^ +help: skipping check that does not even have a feature gate + --> $DIR/nrvo.rs:11:5 + | +LL | init(&mut buf); + | ^^^^^^^^^^^^^^ + +error: `-Zunleash-the-miri-inside-of-you` may not be used to circumvent feature gates, except when testing error paths in the CTFE engine + +error: aborting due to previous error; 1 warning emitted + From b19d5c05929bc4ead9b127b8e28aa6c7a40119da Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 16 May 2020 13:41:30 -0700 Subject: [PATCH 06/11] Name return place in debuginfo if it is a user variable --- src/librustc_codegen_ssa/mir/debuginfo.rs | 3 ++- src/librustc_mir/transform/nrvo.rs | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_codegen_ssa/mir/debuginfo.rs b/src/librustc_codegen_ssa/mir/debuginfo.rs index 5501ed5128d..d166a27b5a9 100644 --- a/src/librustc_codegen_ssa/mir/debuginfo.rs +++ b/src/librustc_codegen_ssa/mir/debuginfo.rs @@ -115,7 +115,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let full_debug_info = bx.sess().opts.debuginfo == DebugInfo::Full; // FIXME(eddyb) maybe name the return place as `_0` or `return`? - if local == mir::RETURN_PLACE { + if local == mir::RETURN_PLACE && !self.mir.local_decls[mir::RETURN_PLACE].is_user_variable() + { return; } diff --git a/src/librustc_mir/transform/nrvo.rs b/src/librustc_mir/transform/nrvo.rs index 557ffe4a6d5..d249eaa2e43 100644 --- a/src/librustc_mir/transform/nrvo.rs +++ b/src/librustc_mir/transform/nrvo.rs @@ -1,3 +1,4 @@ +use rustc_hir::Mutability; use rustc_index::bit_set::HybridBitSet; use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor}; use rustc_middle::mir::{self, BasicBlock, Local, Location}; @@ -61,6 +62,9 @@ impl<'tcx> MirPass<'tcx> for RenameReturnPlace { body.local_decls.pick2_mut(returned_local, mir::RETURN_PLACE); debug_assert_eq!(ret_decl.ty, renamed_decl.ty); ret_decl.clone_from(renamed_decl); + + // The return place is always mutable. + ret_decl.mutability = Mutability::Mut; } } From 8c710118f631af9b075bbc2e24c4a61fd901a130 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 16 May 2020 13:42:41 -0700 Subject: [PATCH 07/11] Disable MIR optimization for alignment codegen tests NRVO optimizes away the locals whose alignment is tested. I don't think this affects the purpose of the test. --- src/test/codegen/align-enum.rs | 2 +- src/test/codegen/align-struct.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/codegen/align-enum.rs b/src/test/codegen/align-enum.rs index 72447fbc079..95ca7cfe750 100644 --- a/src/test/codegen/align-enum.rs +++ b/src/test/codegen/align-enum.rs @@ -1,4 +1,4 @@ -// compile-flags: -C no-prepopulate-passes +// compile-flags: -C no-prepopulate-passes -Z mir-opt-level=0 // ignore-tidy-linelength #![crate_type = "lib"] diff --git a/src/test/codegen/align-struct.rs b/src/test/codegen/align-struct.rs index 5e290323907..cda7235a3d8 100644 --- a/src/test/codegen/align-struct.rs +++ b/src/test/codegen/align-struct.rs @@ -1,4 +1,4 @@ -// compile-flags: -C no-prepopulate-passes +// compile-flags: -C no-prepopulate-passes -Z mir-opt-level=0 // ignore-tidy-linelength #![crate_type = "lib"] From 2fe117061331215e9fa899983f1ca121bba619ec Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 16 May 2020 13:56:51 -0700 Subject: [PATCH 08/11] `ret` has been optimized away in debuginfo test --- src/test/debuginfo/generic-function.rs | 39 ++++++-------------------- 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/src/test/debuginfo/generic-function.rs b/src/test/debuginfo/generic-function.rs index 96f2aa3acf2..f5e34c39119 100644 --- a/src/test/debuginfo/generic-function.rs +++ b/src/test/debuginfo/generic-function.rs @@ -1,5 +1,3 @@ -// ignore-tidy-linelength - // min-lldb-version: 310 // compile-flags:-g @@ -12,31 +10,21 @@ // gdb-check:$1 = 1 // gdb-command:print *t1 // gdb-check:$2 = 2.5 -// gdb-command:print ret -// gdbg-check:$3 = {__0 = {__0 = 1, __1 = 2.5}, __1 = {__0 = 2.5, __1 = 1}} -// gdbr-check:$3 = ((1, 2.5), (2.5, 1)) // gdb-command:continue // gdb-command:print *t0 -// gdb-check:$4 = 3.5 +// gdb-check:$3 = 3.5 // gdb-command:print *t1 -// gdb-check:$5 = 4 -// gdb-command:print ret -// gdbg-check:$6 = {__0 = {__0 = 3.5, __1 = 4}, __1 = {__0 = 4, __1 = 3.5}} -// gdbr-check:$6 = ((3.5, 4), (4, 3.5)) +// gdb-check:$4 = 4 // gdb-command:continue // gdb-command:print *t0 -// gdb-check:$7 = 5 +// gdb-check:$5 = 5 // gdb-command:print *t1 -// gdbg-check:$8 = {a = 6, b = 7.5} -// gdbr-check:$8 = generic_function::Struct {a: 6, b: 7.5} -// gdb-command:print ret -// gdbg-check:$9 = {__0 = {__0 = 5, __1 = {a = 6, b = 7.5}}, __1 = {__0 = {a = 6, b = 7.5}, __1 = 5}} -// gdbr-check:$9 = ((5, generic_function::Struct {a: 6, b: 7.5}), (generic_function::Struct {a: 6, b: 7.5}, 5)) +// gdbg-check:$6 = {a = 6, b = 7.5} +// gdbr-check:$6 = generic_function::Struct {a: 6, b: 7.5} // gdb-command:continue - // === LLDB TESTS ================================================================================== // lldb-command:run @@ -47,31 +35,22 @@ // lldb-command:print *t1 // lldbg-check:[...]$1 = 2.5 // lldbr-check:(f64) *t1 = 2.5 -// lldb-command:print ret -// lldbg-check:[...]$2 = ((1, 2.5), (2.5, 1)) -// lldbr-check:(((i32, f64), (f64, i32))) ret = { = { = 1 = 2.5 } = { = 2.5 = 1 } } // lldb-command:continue // lldb-command:print *t0 -// lldbg-check:[...]$3 = 3.5 +// lldbg-check:[...]$2 = 3.5 // lldbr-check:(f64) *t0 = 3.5 // lldb-command:print *t1 -// lldbg-check:[...]$4 = 4 +// lldbg-check:[...]$3 = 4 // lldbr-check:(u16) *t1 = 4 -// lldb-command:print ret -// lldbg-check:[...]$5 = ((3.5, 4), (4, 3.5)) -// lldbr-check:(((f64, u16), (u16, f64))) ret = { = { = 3.5 = 4 } = { = 4 = 3.5 } } // lldb-command:continue // lldb-command:print *t0 -// lldbg-check:[...]$6 = 5 +// lldbg-check:[...]$4 = 5 // lldbr-check:(i32) *t0 = 5 // lldb-command:print *t1 -// lldbg-check:[...]$7 = Struct { a: 6, b: 7.5 } +// lldbg-check:[...]$5 = Struct { a: 6, b: 7.5 } // lldbr-check:(generic_function::Struct) *t1 = Struct { a: 6, b: 7.5 } -// lldb-command:print ret -// lldbg-check:[...]$8 = ((5, Struct { a: 6, b: 7.5 }), (Struct { a: 6, b: 7.5 }, 5)) -// lldbr-check:(((i32, generic_function::Struct), (generic_function::Struct, i32))) ret = { = { = 5 = Struct { a: 6, b: 7.5 } } = { = Struct { a: 6, b: 7.5 } = 5 } } // lldb-command:continue #![feature(omit_gdb_pretty_printer_section)] From 1deaaa610c949a123751b76302b5cec7e7a1c664 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 17 May 2020 10:23:44 -0700 Subject: [PATCH 09/11] Don't unleash NRVO const-eval test --- .../{miri_unleashed => const-eval}/nrvo.rs | 13 +++++---- src/test/ui/consts/miri_unleashed/nrvo.stderr | 27 ------------------- 2 files changed, 8 insertions(+), 32 deletions(-) rename src/test/ui/consts/{miri_unleashed => const-eval}/nrvo.rs (71%) delete mode 100644 src/test/ui/consts/miri_unleashed/nrvo.stderr diff --git a/src/test/ui/consts/miri_unleashed/nrvo.rs b/src/test/ui/consts/const-eval/nrvo.rs similarity index 71% rename from src/test/ui/consts/miri_unleashed/nrvo.rs rename to src/test/ui/consts/const-eval/nrvo.rs index b7f11024805..1d2c6acc06c 100644 --- a/src/test/ui/consts/miri_unleashed/nrvo.rs +++ b/src/test/ui/consts/const-eval/nrvo.rs @@ -1,20 +1,23 @@ -// compile-flags: -Zunleash-the-miri-inside-of-you // run-pass +// When the NRVO is applied, the return place (`_0`) gets treated like a normal local. For example, +// its address may be taken and it may be written to indirectly. Ensure that MIRI can handle this. + +#![feature(const_mut_refs)] + +#[inline(never)] // Try to ensure that MIR optimizations don't optimize this away. const fn init(buf: &mut [u8; 1024]) { buf[33] = 3; buf[444] = 4; } -const fn nrvo(init: fn(&mut [u8; 1024])) -> [u8; 1024] { +const fn nrvo() -> [u8; 1024] { let mut buf = [0; 1024]; init(&mut buf); buf } -// When the NRVO is applied, the return place (`_0`) gets treated like a normal local. For example, -// its address may be taken and it may be written to indirectly. Ensure that MIRI can handle this. -const BUF: [u8; 1024] = nrvo(init); +const BUF: [u8; 1024] = nrvo(); fn main() { assert_eq!(BUF[33], 3); diff --git a/src/test/ui/consts/miri_unleashed/nrvo.stderr b/src/test/ui/consts/miri_unleashed/nrvo.stderr deleted file mode 100644 index d38f869181b..00000000000 --- a/src/test/ui/consts/miri_unleashed/nrvo.stderr +++ /dev/null @@ -1,27 +0,0 @@ -warning: skipping const checks - | -help: skipping check for `const_mut_refs` feature - --> $DIR/nrvo.rs:5:5 - | -LL | buf[33] = 3; - | ^^^^^^^^^^^ -help: skipping check for `const_mut_refs` feature - --> $DIR/nrvo.rs:6:5 - | -LL | buf[444] = 4; - | ^^^^^^^^^^^^ -help: skipping check for `const_mut_refs` feature - --> $DIR/nrvo.rs:11:10 - | -LL | init(&mut buf); - | ^^^^^^^^ -help: skipping check that does not even have a feature gate - --> $DIR/nrvo.rs:11:5 - | -LL | init(&mut buf); - | ^^^^^^^^^^^^^^ - -error: `-Zunleash-the-miri-inside-of-you` may not be used to circumvent feature gates, except when testing error paths in the CTFE engine - -error: aborting due to previous error; 1 warning emitted - From 856cd6609f42f3dfb71a060205981f075a22c541 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 17 May 2020 10:24:06 -0700 Subject: [PATCH 10/11] Test that NRVO elides the call to `memcpy` --- src/test/codegen/nrvo.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 src/test/codegen/nrvo.rs diff --git a/src/test/codegen/nrvo.rs b/src/test/codegen/nrvo.rs new file mode 100644 index 00000000000..fddb0d1fb3c --- /dev/null +++ b/src/test/codegen/nrvo.rs @@ -0,0 +1,17 @@ +// compile-flags: -O + +#![crate_type = "lib"] + +// Ensure that we do not call `memcpy` for the following function. +// `memset` and `init` should be called directly on the return pointer. +#[no_mangle] +pub fn nrvo(init: fn(&mut [u8; 4096])) -> [u8; 4096] { + // CHECK-LABEL: nrvo + // CHECK: @llvm.memset + // CHECK-NOT: @llvm.memcpy + // CHECK: ret + // CHECK-EMPTY + let mut buf = [0; 4096]; + init(&mut buf); + buf +} From f50986205756075fb05a9371b94facd58cc048a6 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 20 May 2020 22:44:57 -0700 Subject: [PATCH 11/11] Bail out if new return place has different type than old --- src/librustc_mir/transform/nrvo.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/nrvo.rs b/src/librustc_mir/transform/nrvo.rs index d249eaa2e43..941ffa94aa8 100644 --- a/src/librustc_mir/transform/nrvo.rs +++ b/src/librustc_mir/transform/nrvo.rs @@ -44,6 +44,18 @@ impl<'tcx> MirPass<'tcx> for RenameReturnPlace { } }; + // Sometimes, the return place is assigned a local of a different but coercable type, for + // example `&T` instead of `&mut T`. Overwriting the `LocalInfo` for the return place would + // result in it having an incorrect type. Although this doesn't seem to cause a problem in + // codegen, bail out anyways since it happens so rarely. + let ret_ty = body.local_decls[mir::RETURN_PLACE].ty; + let assigned_ty = body.local_decls[returned_local].ty; + if ret_ty != assigned_ty { + debug!("`{:?}` was eligible for NRVO but for type mismatch", src.def_id()); + debug!("typeof(_0) != typeof({:?}); {:?} != {:?}", returned_local, ret_ty, assigned_ty); + return; + } + debug!( "`{:?}` was eligible for NRVO, making {:?} the return place", src.def_id(), @@ -60,7 +72,6 @@ impl<'tcx> MirPass<'tcx> for RenameReturnPlace { // Overwrite the debuginfo of `_0` with that of the renamed local. let (renamed_decl, ret_decl) = body.local_decls.pick2_mut(returned_local, mir::RETURN_PLACE); - debug_assert_eq!(ret_decl.ty, renamed_decl.ty); ret_decl.clone_from(renamed_decl); // The return place is always mutable.