From f0ed021315f579e24b6f12014298c37ee70f6903 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 16 Jun 2020 09:10:19 -0700 Subject: [PATCH 1/8] Use helper method for what types need allocas --- src/librustc_codegen_ssa/mir/analyze.rs | 29 +++++++++++++------------ 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index 2e386c1e5946b..18b915f894707 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -12,7 +12,7 @@ use rustc_middle::mir::visit::{ }; use rustc_middle::mir::{self, Location, TerminatorKind}; use rustc_middle::ty; -use rustc_middle::ty::layout::HasTyCtxt; +use rustc_middle::ty::layout::{HasTyCtxt, TyAndLayout}; use rustc_target::abi::LayoutOf; pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( @@ -27,18 +27,8 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let ty = fx.monomorphize(&decl.ty); debug!("local {:?} has type `{}`", local, ty); let layout = fx.cx.spanned_layout_of(ty, decl.source_info.span); - if fx.cx.is_backend_immediate(layout) { - // These sorts of types are immediates that we can store - // in an Value without an alloca. - } else if fx.cx.is_backend_scalar_pair(layout) { - // We allow pairs and uses of any of their 2 fields. - } else { - // These sorts of types require an alloca. Note that - // is_llvm_immediate() may *still* be true, particularly - // for newtypes, but we currently force some types - // (e.g., structs) into an alloca unconditionally, just so - // that we don't have to deal with having two pathways - // (gep vs extractvalue etc). + + if ty_requires_alloca(&analyzer.fx, layout) { analyzer.not_ssa(local); } } @@ -132,7 +122,7 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { if let mir::ProjectionElem::Field(..) = elem { let layout = cx.spanned_layout_of(base_ty.ty, span); - if cx.is_backend_immediate(layout) || cx.is_backend_scalar_pair(layout) { + if self.ty_requires_alloca(layout) { // Recurse with the same context, instead of `Projection`, // potentially stopping at non-operand projections, // which would trigger `not_ssa` on locals. @@ -446,3 +436,14 @@ pub fn cleanup_kinds(mir: &mir::Body<'_>) -> IndexVec( + fx: &FunctionCx<'a, 'tcx, impl BuilderMethods<'a, 'tcx>>, + ty: TyAndLayout<'tcx>, +) -> bool { + // Currently, this returns `true` for ADTs that are otherwise small enough to qualify. For + // example, `struct Newtype(i32)`. This way, every type has a single way to extract data + // (gep, extractvalue, etc.). + !fx.cx.is_backend_immediate(ty) && !fx.cx.is_backend_scalar_pair(ty) +} From 3e74727a58c8de7627569cd287f69217c3723f10 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 16 Jun 2020 09:44:46 -0700 Subject: [PATCH 2/8] Use `Option` to indicate the first assignment This is the same size as `Location`. Using an invalid location is premature optimization. --- src/librustc_codegen_ssa/mir/analyze.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index 18b915f894707..ac9f229512df3 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -5,7 +5,7 @@ use super::FunctionCx; use crate::traits::*; use rustc_data_structures::graph::dominators::Dominators; use rustc_index::bit_set::BitSet; -use rustc_index::vec::{Idx, IndexVec}; +use rustc_index::vec::IndexVec; use rustc_middle::mir::traversal; use rustc_middle::mir::visit::{ MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor, @@ -40,37 +40,31 @@ struct LocalAnalyzer<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { fx: &'mir FunctionCx<'a, 'tcx, Bx>, dominators: Dominators, non_ssa_locals: BitSet, - // The location of the first visited direct assignment to each - // local, or an invalid location (out of bounds `block` index). - first_assignment: IndexVec, + + /// The location of the first visited direct assignment to each local. + first_assignment: IndexVec>, } impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { fn new(fx: &'mir FunctionCx<'a, 'tcx, Bx>) -> Self { - let invalid_location = mir::BasicBlock::new(fx.mir.basic_blocks().len()).start_location(); let dominators = fx.mir.dominators(); let mut analyzer = LocalAnalyzer { fx, dominators, non_ssa_locals: BitSet::new_empty(fx.mir.local_decls.len()), - first_assignment: IndexVec::from_elem(invalid_location, &fx.mir.local_decls), + first_assignment: IndexVec::from_elem(None, &fx.mir.local_decls), }; // Arguments get assigned to by means of the function being called for arg in fx.mir.args_iter() { - analyzer.first_assignment[arg] = mir::START_BLOCK.start_location(); + analyzer.assign(arg, mir::START_BLOCK.start_location()); } analyzer } fn first_assignment(&self, local: mir::Local) -> Option { - let location = self.first_assignment[local]; - if location.block.index() < self.fx.mir.basic_blocks().len() { - Some(location) - } else { - None - } + self.first_assignment[local] } fn not_ssa(&mut self, local: mir::Local) { @@ -79,10 +73,10 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { } fn assign(&mut self, local: mir::Local, location: Location) { - if self.first_assignment(local).is_some() { + if self.first_assignment[local].is_some() { self.not_ssa(local); } else { - self.first_assignment[local] = location; + self.first_assignment[local] = Some(location); } } From e0fdf7c7c7d3aebef0860bd2999c94527eb69385 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 28 Jun 2020 15:24:30 -0700 Subject: [PATCH 3/8] Refactor `LocalAnalyzer` to use `visit_projection_elem` --- src/librustc_codegen_ssa/mir/analyze.rs | 247 ++++++++++++------------ 1 file changed, 126 insertions(+), 121 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index ac9f229512df3..f549c0ff54dd5 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -7,9 +7,7 @@ use rustc_data_structures::graph::dominators::Dominators; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; use rustc_middle::mir::traversal; -use rustc_middle::mir::visit::{ - MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor, -}; +use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::{self, Location, TerminatorKind}; use rustc_middle::ty; use rustc_middle::ty::layout::{HasTyCtxt, TyAndLayout}; @@ -21,7 +19,13 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let mir = fx.mir; let mut analyzer = LocalAnalyzer::new(fx); - analyzer.visit_body(&mir); + for (block, data) in traversal::reverse_postorder(mir) { + analyzer.visit_basic_block_data(block, data); + } + + for debuginfo in mir.var_debug_info.iter() { + analyzer.visit_var_debug_info(debuginfo); + } for (local, decl) in mir.local_decls.iter_enumerated() { let ty = fx.monomorphize(&decl.ty); @@ -36,6 +40,12 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( analyzer.non_ssa_locals } +#[derive(Default, PartialEq, Eq)] +struct PlaceInfo { + has_disqualifying_projection: bool, + has_deref_projection: bool, +} + struct LocalAnalyzer<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { fx: &'mir FunctionCx<'a, 'tcx, Bx>, dominators: Dominators, @@ -43,6 +53,8 @@ struct LocalAnalyzer<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { /// The location of the first visited direct assignment to each local. first_assignment: IndexVec>, + + place_info: PlaceInfo, } impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { @@ -53,6 +65,7 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { dominators, non_ssa_locals: BitSet::new_empty(fx.mir.local_decls.len()), first_assignment: IndexVec::from_elem(None, &fx.mir.local_decls), + place_info: PlaceInfo::default(), }; // Arguments get assigned to by means of the function being called @@ -79,118 +92,6 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { self.first_assignment[local] = Some(location); } } - - fn process_place( - &mut self, - place_ref: &mir::PlaceRef<'tcx>, - context: PlaceContext, - location: Location, - ) { - let cx = self.fx.cx; - - if let &[ref proj_base @ .., elem] = place_ref.projection { - let mut base_context = if context.is_mutating_use() { - PlaceContext::MutatingUse(MutatingUseContext::Projection) - } else { - PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) - }; - - // Allow uses of projections that are ZSTs or from scalar fields. - let is_consume = match context { - PlaceContext::NonMutatingUse( - NonMutatingUseContext::Copy | NonMutatingUseContext::Move, - ) => true, - _ => false, - }; - if is_consume { - let base_ty = - mir::Place::ty_from(place_ref.local, proj_base, self.fx.mir, cx.tcx()); - let base_ty = self.fx.monomorphize(&base_ty); - - // ZSTs don't require any actual memory access. - let elem_ty = base_ty.projection_ty(cx.tcx(), self.fx.monomorphize(&elem)).ty; - let span = self.fx.mir.local_decls[place_ref.local].source_info.span; - if cx.spanned_layout_of(elem_ty, span).is_zst() { - return; - } - - if let mir::ProjectionElem::Field(..) = elem { - let layout = cx.spanned_layout_of(base_ty.ty, span); - if self.ty_requires_alloca(layout) { - // Recurse with the same context, instead of `Projection`, - // potentially stopping at non-operand projections, - // which would trigger `not_ssa` on locals. - base_context = context; - } - } - } - - if let mir::ProjectionElem::Deref = elem { - // Deref projections typically only read the pointer. - // (the exception being `VarDebugInfo` contexts, handled below) - base_context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy); - - // Indirect debuginfo requires going through memory, that only - // the debugger accesses, following our emitted DWARF pointer ops. - // - // FIXME(eddyb) Investigate the possibility of relaxing this, but - // note that `llvm.dbg.declare` *must* be used for indirect places, - // even if we start using `llvm.dbg.value` for all other cases, - // as we don't necessarily know when the value changes, but only - // where it lives in memory. - // - // It's possible `llvm.dbg.declare` could support starting from - // a pointer that doesn't point to an `alloca`, but this would - // only be useful if we know the pointer being `Deref`'d comes - // from an immutable place, and if `llvm.dbg.declare` calls - // must be at the very start of the function, then only function - // arguments could contain such pointers. - if context == PlaceContext::NonUse(NonUseContext::VarDebugInfo) { - // We use `NonUseContext::VarDebugInfo` for the base, - // which might not force the base local to memory, - // so we have to do it manually. - self.visit_local(&place_ref.local, context, location); - } - } - - // `NonUseContext::VarDebugInfo` needs to flow all the - // way down to the base local (see `visit_local`). - if context == PlaceContext::NonUse(NonUseContext::VarDebugInfo) { - base_context = context; - } - - self.process_place( - &mir::PlaceRef { local: place_ref.local, projection: proj_base }, - base_context, - location, - ); - // HACK(eddyb) this emulates the old `visit_projection_elem`, this - // entire `visit_place`-like `process_place` method should be rewritten, - // now that we have moved to the "slice of projections" representation. - if let mir::ProjectionElem::Index(local) = elem { - self.visit_local( - &local, - PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy), - location, - ); - } - } else { - // FIXME this is super_place code, is repeated here to avoid cloning place or changing - // visit_place API - let mut context = context; - - if !place_ref.projection.is_empty() { - context = if context.is_mutating_use() { - PlaceContext::MutatingUse(MutatingUseContext::Projection) - } else { - PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) - }; - } - - self.visit_local(&place_ref.local, context, location); - self.visit_projection(place_ref.local, place_ref.projection, context, location); - } - } } impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> @@ -247,7 +148,45 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> fn visit_place(&mut self, place: &mir::Place<'tcx>, context: PlaceContext, location: Location) { debug!("visit_place(place={:?}, context={:?})", place, context); - self.process_place(&place.as_ref(), context, location); + + // Except for `VarDebugInfo`, non-uses do not force locals onto the stack. + // + // `VarDebugInfo` is handled in `visit_var_debug_info`. + if !context.is_use() { + return; + } + + let mir::Place { local, projection } = *place; + + // Reads from ZSTs do not require memory accesses. + if is_consume(context) { + let ty = place.ty(self.fx.mir, self.fx.cx.tcx()).ty; + let ty = self.fx.monomorphize(&ty); + let span = self.fx.mir.local_decls[local].source_info.span; + if self.fx.cx.spanned_layout_of(ty, span).is_zst() { + return; + } + } + + assert!(self.place_info == PlaceInfo::default(), "`visit_place` should not recurse"); + self.visit_projection(local, projection, context, location); + + let PlaceInfo { has_disqualifying_projection, has_deref_projection } = + std::mem::take(&mut self.place_info); + + if has_disqualifying_projection { + self.not_ssa(local); + return; + } + + // Treat a `Deref` of a local as a `Copy` of that local. + let context = if has_deref_projection { + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) + } else { + context + }; + + self.visit_local(&local, context, location); } fn visit_local(&mut self, &local: &mir::Local, context: PlaceContext, location: Location) { @@ -277,20 +216,23 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } + PlaceContext::MutatingUse(MutatingUseContext::Projection) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) => { + unreachable!("We always use the original context from `visit_place`") + } + PlaceContext::MutatingUse( MutatingUseContext::Store | MutatingUseContext::AsmOutput | MutatingUseContext::Borrow - | MutatingUseContext::AddressOf - | MutatingUseContext::Projection, + | MutatingUseContext::AddressOf, ) | PlaceContext::NonMutatingUse( NonMutatingUseContext::Inspect | NonMutatingUseContext::SharedBorrow | NonMutatingUseContext::UniqueBorrow | NonMutatingUseContext::ShallowBorrow - | NonMutatingUseContext::AddressOf - | NonMutatingUseContext::Projection, + | NonMutatingUseContext::AddressOf, ) => { self.not_ssa(local); } @@ -306,6 +248,62 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } } + + fn visit_projection_elem( + &mut self, + local: mir::Local, + proj_base: &[mir::PlaceElem<'tcx>], + elem: mir::PlaceElem<'tcx>, + context: PlaceContext, + location: Location, + ) { + self.super_projection_elem(local, proj_base, elem, context, location); + + // Projections like `(*x)[12]` are allowed but not `*(x[12])`. + if let mir::PlaceElem::Deref = elem { + self.place_info.has_disqualifying_projection = false; + self.place_info.has_deref_projection = true; + return; + } + + if !is_consume(context) { + self.place_info.has_disqualifying_projection = true; + return; + } + + if let mir::ProjectionElem::Field(..) = elem { + let base_ty = mir::Place::ty_from(local, proj_base, self.fx.mir, self.fx.cx.tcx()); + let base_ty = self.fx.monomorphize(&base_ty); + let span = self.fx.mir.local_decls[local].source_info.span; + let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span); + if !ty_requires_alloca(self.fx, layout) { + return; + } + } + + self.place_info.has_disqualifying_projection = true; + } + + fn visit_var_debug_info(&mut self, var_debug_info: &mir::VarDebugInfo<'tcx>) { + // Indirect debuginfo requires going through memory, that only + // the debugger accesses, following our emitted DWARF pointer ops. + // + // FIXME(eddyb) Investigate the possibility of relaxing this, but + // note that `llvm.dbg.declare` *must* be used for indirect places, + // even if we start using `llvm.dbg.value` for all other cases, + // as we don't necessarily know when the value changes, but only + // where it lives in memory. + // + // It's possible `llvm.dbg.declare` could support starting from + // a pointer that doesn't point to an `alloca`, but this would + // only be useful if we know the pointer being `Deref`'d comes + // from an immutable place, and if `llvm.dbg.declare` calls + // must be at the very start of the function, then only function + // arguments could contain such pointers. + if var_debug_info.place.is_indirect() { + self.not_ssa(var_debug_info.place.local); + } + } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -441,3 +439,10 @@ fn ty_requires_alloca<'a, 'tcx>( // (gep, extractvalue, etc.). !fx.cx.is_backend_immediate(ty) && !fx.cx.is_backend_scalar_pair(ty) } + +fn is_consume(context: PlaceContext) -> bool { + matches!( + context, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy | NonMutatingUseContext::Move) + ) +} From da03a98bf236f74eea0939f6be28ec73600f6314 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 15 Jul 2020 11:32:05 -0700 Subject: [PATCH 4/8] Use loop instead of visitor to check for invalid projections --- src/librustc_codegen_ssa/mir/analyze.rs | 96 ++++++++++--------------- 1 file changed, 38 insertions(+), 58 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index f549c0ff54dd5..f3411998248b8 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -16,6 +16,8 @@ use rustc_target::abi::LayoutOf; pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( fx: &FunctionCx<'a, 'tcx, Bx>, ) -> BitSet { + trace!("non_ssa_locals({:?})", fx.instance.def_id()); + let mir = fx.mir; let mut analyzer = LocalAnalyzer::new(fx); @@ -40,12 +42,6 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( analyzer.non_ssa_locals } -#[derive(Default, PartialEq, Eq)] -struct PlaceInfo { - has_disqualifying_projection: bool, - has_deref_projection: bool, -} - struct LocalAnalyzer<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { fx: &'mir FunctionCx<'a, 'tcx, Bx>, dominators: Dominators, @@ -53,8 +49,6 @@ struct LocalAnalyzer<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { /// The location of the first visited direct assignment to each local. first_assignment: IndexVec>, - - place_info: PlaceInfo, } impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { @@ -65,7 +59,6 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { dominators, non_ssa_locals: BitSet::new_empty(fx.mir.local_decls.len()), first_assignment: IndexVec::from_elem(None, &fx.mir.local_decls), - place_info: PlaceInfo::default(), }; // Arguments get assigned to by means of the function being called @@ -146,7 +139,12 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> self.super_terminator(terminator, location); } - fn visit_place(&mut self, place: &mir::Place<'tcx>, context: PlaceContext, location: Location) { + fn visit_place( + &mut self, + place: &mir::Place<'tcx>, + mut context: PlaceContext, + location: Location, + ) { debug!("visit_place(place={:?}, context={:?})", place, context); // Except for `VarDebugInfo`, non-uses do not force locals onto the stack. @@ -158,7 +156,8 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> let mir::Place { local, projection } = *place; - // Reads from ZSTs do not require memory accesses. + // Reads from ZSTs do not require memory accesses and do not count when determining what + // needs to live on the stack. if is_consume(context) { let ty = place.ty(self.fx.mir, self.fx.cx.tcx()).ty; let ty = self.fx.monomorphize(&ty); @@ -168,24 +167,40 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } - assert!(self.place_info == PlaceInfo::default(), "`visit_place` should not recurse"); - self.visit_projection(local, projection, context, location); + let mut has_disqualifying_projection = false; - let PlaceInfo { has_disqualifying_projection, has_deref_projection } = - std::mem::take(&mut self.place_info); + let mut projection: &[_] = projection.as_ref(); + while let [ref proj_base @ .., elem] = *projection { + projection = proj_base; + self.super_projection_elem(local, proj_base, elem, context, location); + + // Projections like `(*x)[12]` are allowed but not `*(x[12])`, since a `Deref` of a + // local acts like a `Copy` of that local. + if let mir::PlaceElem::Deref = elem { + has_disqualifying_projection = false; + context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy); + continue; + } + + // Ignoring `Deref`s, the only allowed projections are reads of scalar fields. + if is_consume(context) && matches!(elem, mir::ProjectionElem::Field(..)) { + let base_ty = mir::Place::ty_from(local, proj_base, self.fx.mir, self.fx.cx.tcx()); + let base_ty = self.fx.monomorphize(&base_ty); + let span = self.fx.mir.local_decls[local].source_info.span; + let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span); + + if !ty_requires_alloca(self.fx, layout) { + continue; + } + } + + has_disqualifying_projection = true; + } if has_disqualifying_projection { self.not_ssa(local); - return; } - // Treat a `Deref` of a local as a `Copy` of that local. - let context = if has_deref_projection { - PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) - } else { - context - }; - self.visit_local(&local, context, location); } @@ -249,41 +264,6 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } - fn visit_projection_elem( - &mut self, - local: mir::Local, - proj_base: &[mir::PlaceElem<'tcx>], - elem: mir::PlaceElem<'tcx>, - context: PlaceContext, - location: Location, - ) { - self.super_projection_elem(local, proj_base, elem, context, location); - - // Projections like `(*x)[12]` are allowed but not `*(x[12])`. - if let mir::PlaceElem::Deref = elem { - self.place_info.has_disqualifying_projection = false; - self.place_info.has_deref_projection = true; - return; - } - - if !is_consume(context) { - self.place_info.has_disqualifying_projection = true; - return; - } - - if let mir::ProjectionElem::Field(..) = elem { - let base_ty = mir::Place::ty_from(local, proj_base, self.fx.mir, self.fx.cx.tcx()); - let base_ty = self.fx.monomorphize(&base_ty); - let span = self.fx.mir.local_decls[local].source_info.span; - let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span); - if !ty_requires_alloca(self.fx, layout) { - return; - } - } - - self.place_info.has_disqualifying_projection = true; - } - fn visit_var_debug_info(&mut self, var_debug_info: &mir::VarDebugInfo<'tcx>) { // Indirect debuginfo requires going through memory, that only // the debugger accesses, following our emitted DWARF pointer ops. From 7fcd4cb270e14a5710b5b9edb86a9bc42ce38df8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 15 Jul 2020 11:32:29 -0700 Subject: [PATCH 5/8] `first_assignment` no longer needs a getter --- src/librustc_codegen_ssa/mir/analyze.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index f3411998248b8..66bb1220671e9 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -69,10 +69,6 @@ impl> LocalAnalyzer<'mir, 'a, 'tcx, Bx> { analyzer } - fn first_assignment(&self, local: mir::Local) -> Option { - self.first_assignment[local] - } - fn not_ssa(&mut self, local: mir::Local) { debug!("marking {:?} as non-SSA", local); self.non_ssa_locals.insert(local); @@ -220,7 +216,7 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> // optimizations) require locals to be in (uninitialized) memory. // N.B., there can be uninitialized reads of a local visited after // an assignment to that local, if they happen on disjoint paths. - let ssa_read = match self.first_assignment(local) { + let ssa_read = match self.first_assignment[local] { Some(assignment_location) => { assignment_location.dominates(location, &self.dominators) } From a4d0bed3f7c266a7e794628090fba58e36c083d6 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 15 Jul 2020 11:32:37 -0700 Subject: [PATCH 6/8] Ignore `VarDebugInfo` when forcing locals on the stack --- src/librustc_codegen_ssa/mir/analyze.rs | 29 +------------------------ 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index 66bb1220671e9..ddbab93196b55 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -25,10 +25,6 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( analyzer.visit_basic_block_data(block, data); } - for debuginfo in mir.var_debug_info.iter() { - analyzer.visit_var_debug_info(debuginfo); - } - for (local, decl) in mir.local_decls.iter_enumerated() { let ty = fx.monomorphize(&decl.ty); debug!("local {:?} has type `{}`", local, ty); @@ -143,9 +139,7 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> ) { debug!("visit_place(place={:?}, context={:?})", place, context); - // Except for `VarDebugInfo`, non-uses do not force locals onto the stack. - // - // `VarDebugInfo` is handled in `visit_var_debug_info`. + // Non-uses do not force locals onto the stack. if !context.is_use() { return; } @@ -259,27 +253,6 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } } - - fn visit_var_debug_info(&mut self, var_debug_info: &mir::VarDebugInfo<'tcx>) { - // Indirect debuginfo requires going through memory, that only - // the debugger accesses, following our emitted DWARF pointer ops. - // - // FIXME(eddyb) Investigate the possibility of relaxing this, but - // note that `llvm.dbg.declare` *must* be used for indirect places, - // even if we start using `llvm.dbg.value` for all other cases, - // as we don't necessarily know when the value changes, but only - // where it lives in memory. - // - // It's possible `llvm.dbg.declare` could support starting from - // a pointer that doesn't point to an `alloca`, but this would - // only be useful if we know the pointer being `Deref`'d comes - // from an immutable place, and if `llvm.dbg.declare` calls - // must be at the very start of the function, then only function - // arguments could contain such pointers. - if var_debug_info.place.is_indirect() { - self.not_ssa(var_debug_info.place.local); - } - } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] From f74e65ccbdd07cea6886adde97b2fb12e17618d5 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 15 Jul 2020 11:35:01 -0700 Subject: [PATCH 7/8] Remove outdated comment --- src/librustc_codegen_ssa/mir/analyze.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index ddbab93196b55..e6b81fcfe6e2c 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -383,9 +383,6 @@ fn ty_requires_alloca<'a, 'tcx>( fx: &FunctionCx<'a, 'tcx, impl BuilderMethods<'a, 'tcx>>, ty: TyAndLayout<'tcx>, ) -> bool { - // Currently, this returns `true` for ADTs that are otherwise small enough to qualify. For - // example, `struct Newtype(i32)`. This way, every type has a single way to extract data - // (gep, extractvalue, etc.). !fx.cx.is_backend_immediate(ty) && !fx.cx.is_backend_scalar_pair(ty) } From 277d4ca4101003a7d9394c06e3fbea31a6480258 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 17 Aug 2020 13:07:21 -0700 Subject: [PATCH 8/8] Rewrite projection loop using notes from review --- src/librustc_codegen_ssa/mir/analyze.rs | 72 +++++++++++++++---------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/analyze.rs b/src/librustc_codegen_ssa/mir/analyze.rs index e6b81fcfe6e2c..8458e6624b268 100644 --- a/src/librustc_codegen_ssa/mir/analyze.rs +++ b/src/librustc_codegen_ssa/mir/analyze.rs @@ -137,18 +137,20 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> mut context: PlaceContext, location: Location, ) { - debug!("visit_place(place={:?}, context={:?})", place, context); + let mir::Place { local, projection } = *place; + + self.super_projection(local, projection, context, location); // Non-uses do not force locals onto the stack. if !context.is_use() { return; } - let mir::Place { local, projection } = *place; + let is_consume = is_consume(context); // Reads from ZSTs do not require memory accesses and do not count when determining what // needs to live on the stack. - if is_consume(context) { + if is_consume { let ty = place.ty(self.fx.mir, self.fx.cx.tcx()).ty; let ty = self.fx.monomorphize(&ty); let span = self.fx.mir.local_decls[local].source_info.span; @@ -157,41 +159,53 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> } } - let mut has_disqualifying_projection = false; - - let mut projection: &[_] = projection.as_ref(); - while let [ref proj_base @ .., elem] = *projection { - projection = proj_base; - self.super_projection_elem(local, proj_base, elem, context, location); - - // Projections like `(*x)[12]` are allowed but not `*(x[12])`, since a `Deref` of a - // local acts like a `Copy` of that local. - if let mir::PlaceElem::Deref = elem { - has_disqualifying_projection = false; - context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy); - continue; - } + let is_indirect = place.is_indirect(); + if is_indirect { + context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy); + } - // Ignoring `Deref`s, the only allowed projections are reads of scalar fields. - if is_consume(context) && matches!(elem, mir::ProjectionElem::Field(..)) { - let base_ty = mir::Place::ty_from(local, proj_base, self.fx.mir, self.fx.cx.tcx()); - let base_ty = self.fx.monomorphize(&base_ty); - let span = self.fx.mir.local_decls[local].source_info.span; - let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span); + self.visit_local(&local, context, location); - if !ty_requires_alloca(self.fx, layout) { - continue; - } + // In any context besides a simple read or pointer deref, any projections whatsoever force + // a value onto the stack. + if !is_consume && !is_indirect { + if !projection.is_empty() { + self.not_ssa(local); } - has_disqualifying_projection = true; + return; } - if has_disqualifying_projection { + // Only projections inside a `Deref` can disqualify a local from being placed on the stack. + // In other words, `(*x)[idx]` does not disqualify `x` but `*(x[idx])` does. + let first_deref = projection.iter().position(|elem| matches!(elem, mir::PlaceElem::Deref)); + let projections_on_base_local = &projection[..first_deref.unwrap_or(projection.len())]; + + // Only field projections are allowed. We check this before checking the layout of each + // projection below since computing layouts is relatively expensive. + if !projections_on_base_local.iter().all(|elem| matches!(elem, mir::PlaceElem::Field(..))) { self.not_ssa(local); + return; } - self.visit_local(&local, context, location); + // Ensure that each field being projected through is handled correctly. + for (i, elem) in projections_on_base_local.iter().enumerate() { + assert!(matches!(elem, mir::PlaceElem::Field(..))); + + // The inclusive range here means we check every projection prefix but the empty one. + // This is okay since the type of each local is checked in `non_ssa_locals`. + let base = &projection[..=i]; + + let base_ty = mir::Place::ty_from(local, base, self.fx.mir, self.fx.cx.tcx()); + let base_ty = self.fx.monomorphize(&base_ty); + let span = self.fx.mir.local_decls[local].source_info.span; + let layout = self.fx.cx.spanned_layout_of(base_ty.ty, span); + + if ty_requires_alloca(self.fx, layout) { + self.not_ssa(local); + return; + } + } } fn visit_local(&mut self, &local: &mir::Local, context: PlaceContext, location: Location) {