From e07aecde4896c3752256eb8baded936b07fee4d0 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 10:38:54 +0530 Subject: [PATCH 1/8] Make make_clone_call take a Place argument --- src/librustc_mir/shim.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index c206d0ea9b5fd..26825eabb3e7d 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -407,8 +407,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { ty: Ty<'tcx>, rcvr_field: Place<'tcx>, next: BasicBlock, - cleanup: BasicBlock - ) -> Place<'tcx> { + cleanup: BasicBlock, + place: Place<'tcx> + ) { let tcx = self.tcx; let substs = Substs::for_item( @@ -439,8 +440,6 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { }) ); - let loc = self.make_place(Mutability::Not, ty); - // `let ref_loc: &ty = &rcvr_field;` let statement = self.make_statement( StatementKind::Assign( @@ -453,11 +452,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(vec![statement], TerminatorKind::Call { func, args: vec![Operand::Move(ref_loc)], - destination: Some((loc.clone(), next)), + destination: Some((place, next)), cleanup: Some(cleanup), }, false); - - loc } fn loop_header( @@ -540,7 +537,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { // `let cloned = Clone::clone(rcvr[beg])`; // Goto #3 if ok, #5 if unwinding happens. let rcvr_field = rcvr.clone().index(beg); - let cloned = self.make_clone_call(ty, rcvr_field, BasicBlock::new(3), BasicBlock::new(5)); + let cloned = self.make_place(Mutability::Not, ty); + self.make_clone_call(ty, rcvr_field, BasicBlock::new(3), + BasicBlock::new(5), cloned.clone()); // BB #3 // `ret[beg] = cloned;` @@ -638,16 +637,18 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { for (i, ity) in tys.iter().enumerate() { let rcvr_field = rcvr.clone().field(Field::new(i), *ity); + let place = self.make_place(Mutability::Not, ity); + returns.push(place.clone()); + // BB #(2i) // `returns[i] = Clone::clone(&rcvr.i);` // Goto #(2i + 2) if ok, #(2i + 1) if unwinding happens. - returns.push( - self.make_clone_call( - *ity, - rcvr_field, - BasicBlock::new(2 * i + 2), - BasicBlock::new(2 * i + 1), - ) + self.make_clone_call( + *ity, + rcvr_field, + BasicBlock::new(2 * i + 2), + BasicBlock::new(2 * i + 1), + place ); // BB #(2i + 1) (cleanup) From 7fd3c27345c69c5f0eac5372467427408293c045 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 10:51:48 +0530 Subject: [PATCH 2/8] Write directly to the RETURN_PLACE in tuple_like_shim --- src/librustc_mir/shim.rs | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 26825eabb3e7d..cc351368233c5 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -408,7 +408,7 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { rcvr_field: Place<'tcx>, next: BasicBlock, cleanup: BasicBlock, - place: Place<'tcx> + dest: Place<'tcx> ) { let tcx = self.tcx; @@ -452,7 +452,7 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(vec![statement], TerminatorKind::Call { func, args: vec![Operand::Move(ref_loc)], - destination: Some((place, next)), + destination: Some((dest, next)), cleanup: Some(cleanup), }, false); } @@ -633,12 +633,13 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { let rcvr = Place::Local(Local::new(1+0)).deref(); - let mut returns = Vec::new(); + let mut previous_place = None; + let return_place = Place::Local(RETURN_PLACE); for (i, ity) in tys.iter().enumerate() { - let rcvr_field = rcvr.clone().field(Field::new(i), *ity); + let field = Field::new(i); + let rcvr_field = rcvr.clone().field(field, *ity); - let place = self.make_place(Mutability::Not, ity); - returns.push(place.clone()); + let place = return_place.clone().field(field, *ity); // BB #(2i) // `returns[i] = Clone::clone(&rcvr.i);` @@ -648,34 +649,26 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { rcvr_field, BasicBlock::new(2 * i + 2), BasicBlock::new(2 * i + 1), - place + place.clone() ); // BB #(2i + 1) (cleanup) - if i == 0 { - // Nothing to drop, just resume. - self.block(vec![], TerminatorKind::Resume, true); - } else { + if let Some(previous_place) = previous_place.take() { // Drop previous field and goto previous cleanup block. self.block(vec![], TerminatorKind::Drop { - location: returns[i - 1].clone(), + location: previous_place, target: BasicBlock::new(2 * i - 1), unwind: None, }, true); + } else { + // Nothing to drop, just resume. + self.block(vec![], TerminatorKind::Resume, true); } + + previous_place = Some(place); } - // `return kind(returns[0], returns[1], ..., returns[tys.len() - 1]);` - let ret_statement = self.make_statement( - StatementKind::Assign( - Place::Local(RETURN_PLACE), - Rvalue::Aggregate( - box kind, - returns.into_iter().map(Operand::Move).collect() - ) - ) - ); - self.block(vec![ret_statement], TerminatorKind::Return, false); + self.block(vec![], TerminatorKind::Return, false); } } From c6140970f5fcb161c5cd72d0a1b842783b053e55 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 11:55:50 +0530 Subject: [PATCH 3/8] Remove AggregateKind argument from tuple_like_shim --- src/librustc_mir/shim.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index cc351368233c5..28bdaa41297b4 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -305,11 +305,10 @@ fn build_clone_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, } ty::TyClosure(def_id, substs) => { builder.tuple_like_shim( - &substs.upvar_tys(def_id, tcx).collect::>(), - AggregateKind::Closure(def_id, substs) + &substs.upvar_tys(def_id, tcx).collect::>() ) } - ty::TyTuple(tys, _) => builder.tuple_like_shim(&**tys, AggregateKind::Tuple), + ty::TyTuple(tys, _) => builder.tuple_like_shim(&**tys), _ => { bug!("clone shim for `{:?}` which is not `Copy` and is not an aggregate", self_ty) } @@ -625,12 +624,7 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(vec![], TerminatorKind::Resume, true); } - fn tuple_like_shim(&mut self, tys: &[ty::Ty<'tcx>], kind: AggregateKind<'tcx>) { - match kind { - AggregateKind::Tuple | AggregateKind::Closure(..) => (), - _ => bug!("only tuples and closures are accepted"), - }; - + fn tuple_like_shim(&mut self, tys: &[ty::Ty<'tcx>]) { let rcvr = Place::Local(Local::new(1+0)).deref(); let mut previous_place = None; From 48a7a1f5e954c1fc6693e0fddc793bf6b3e2bb25 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 29 Jan 2018 14:11:32 +0530 Subject: [PATCH 4/8] Document the index used in AggregateKind::Adt --- src/librustc/mir/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 3b644aa13f321..e48d21a3e17d1 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1515,8 +1515,8 @@ pub enum AggregateKind<'tcx> { Array(Ty<'tcx>), Tuple, - /// The second field is variant number (discriminant), it's equal - /// to 0 for struct and union expressions. The fourth field is + /// The second field is the variant index. It's equal to 0 for struct + /// and union expressions. The fourth field is /// active field number and is present only for union expressions /// -- e.g. for a union expression `SomeUnion { c: .. }`, the /// active field index would identity the field `c` From ef4f4864f166e4f148d5b903bc928a1bcb63ead5 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 17:24:56 +0530 Subject: [PATCH 5/8] Use dest,src ordering for make_clone_call --- src/librustc_mir/shim.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 28bdaa41297b4..36dd359b20c86 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -403,11 +403,11 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { fn make_clone_call( &mut self, + dest: Place<'tcx>, + src: Place<'tcx>, ty: Ty<'tcx>, - rcvr_field: Place<'tcx>, next: BasicBlock, - cleanup: BasicBlock, - dest: Place<'tcx> + cleanup: BasicBlock ) { let tcx = self.tcx; @@ -439,11 +439,11 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { }) ); - // `let ref_loc: &ty = &rcvr_field;` + // `let ref_loc: &ty = &src;` let statement = self.make_statement( StatementKind::Assign( ref_loc.clone(), - Rvalue::Ref(tcx.types.re_erased, BorrowKind::Shared, rcvr_field) + Rvalue::Ref(tcx.types.re_erased, BorrowKind::Shared, src) ) ); @@ -537,8 +537,8 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { // Goto #3 if ok, #5 if unwinding happens. let rcvr_field = rcvr.clone().index(beg); let cloned = self.make_place(Mutability::Not, ty); - self.make_clone_call(ty, rcvr_field, BasicBlock::new(3), - BasicBlock::new(5), cloned.clone()); + self.make_clone_call(cloned.clone(), rcvr_field, ty, BasicBlock::new(3), + BasicBlock::new(5)); // BB #3 // `ret[beg] = cloned;` @@ -639,11 +639,11 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { // `returns[i] = Clone::clone(&rcvr.i);` // Goto #(2i + 2) if ok, #(2i + 1) if unwinding happens. self.make_clone_call( - *ity, + place.clone(), rcvr_field, + *ity, BasicBlock::new(2 * i + 2), BasicBlock::new(2 * i + 1), - place.clone() ); // BB #(2i + 1) (cleanup) From f97629160fe944cd9185ba180221d19f66126e2f Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 14:47:25 +0530 Subject: [PATCH 6/8] Correctly subst the fn_sig so that we get the correct types Otherwise we get random TySelfs there, which means operations on RETURN_PLACE end up breaking down badly. --- src/librustc_mir/shim.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 36dd359b20c86..54aed69a315d2 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -294,7 +294,7 @@ fn build_clone_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, { debug!("build_clone_shim(def_id={:?})", def_id); - let mut builder = CloneShimBuilder::new(tcx, def_id); + let mut builder = CloneShimBuilder::new(tcx, def_id, self_ty); let is_copy = !self_ty.moves_by_default(tcx, tcx.param_env(def_id), builder.span); match self_ty.sty { @@ -327,8 +327,14 @@ struct CloneShimBuilder<'a, 'tcx: 'a> { } impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { - fn new(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Self { - let sig = tcx.fn_sig(def_id); + fn new(tcx: TyCtxt<'a, 'tcx, 'tcx>, + def_id: DefId, + self_ty: Ty<'tcx>) -> Self { + // we must subst the self_ty because it's + // otherwise going to be TySelf and we can't index + // or access fields of a Place of type TySelf. + let substs = tcx.mk_substs_trait(self_ty, &[]); + let sig = tcx.fn_sig(def_id).subst(tcx, substs); let sig = tcx.erase_late_bound_regions(&sig); let span = tcx.def_span(def_id); From dfd244d952676009e807dca6a9bfdb378b7db72f Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 17:30:15 +0530 Subject: [PATCH 7/8] Eliminate ret_field and ret intermediates in array clone shim --- src/librustc_mir/shim.rs | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 54aed69a315d2..58914097b43fb 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -505,11 +505,11 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { fn array_shim(&mut self, ty: Ty<'tcx>, len: u64) { let tcx = self.tcx; let span = self.span; - let rcvr = Place::Local(Local::new(1+0)).deref(); + let src = Place::Local(Local::new(1+0)).deref(); + let dest = Place::Local(RETURN_PLACE); let beg = self.local_decls.push(temp_decl(Mutability::Mut, tcx.types.usize, span)); let end = self.make_place(Mutability::Not, tcx.types.usize); - let ret = self.make_place(Mutability::Mut, tcx.mk_array(ty, len)); // BB #0 // `let mut beg = 0;` @@ -539,25 +539,17 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.loop_header(Place::Local(beg), end, BasicBlock::new(2), BasicBlock::new(4), false); // BB #2 - // `let cloned = Clone::clone(rcvr[beg])`; + // `dest[i] = Clone::clone(src[beg])`; // Goto #3 if ok, #5 if unwinding happens. - let rcvr_field = rcvr.clone().index(beg); - let cloned = self.make_place(Mutability::Not, ty); - self.make_clone_call(cloned.clone(), rcvr_field, ty, BasicBlock::new(3), + let dest_field = dest.clone().index(beg); + let src_field = src.clone().index(beg); + self.make_clone_call(dest_field, src_field, ty, BasicBlock::new(3), BasicBlock::new(5)); // BB #3 - // `ret[beg] = cloned;` // `beg = beg + 1;` // `goto #1`; - let ret_field = ret.clone().index(beg); let statements = vec![ - self.make_statement( - StatementKind::Assign( - ret_field, - Rvalue::Use(Operand::Move(cloned)) - ) - ), self.make_statement( StatementKind::Assign( Place::Local(beg), @@ -572,14 +564,8 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(statements, TerminatorKind::Goto { target: BasicBlock::new(1) }, false); // BB #4 - // `return ret;` - let ret_statement = self.make_statement( - StatementKind::Assign( - Place::Local(RETURN_PLACE), - Rvalue::Use(Operand::Move(ret.clone())), - ) - ); - self.block(vec![ret_statement], TerminatorKind::Return, false); + // `return dest;` + self.block(vec![], TerminatorKind::Return, false); // BB #5 (cleanup) // `let end = beg;` @@ -604,9 +590,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { BasicBlock::new(7), BasicBlock::new(9), true); // BB #7 (cleanup) - // `drop(ret[beg])`; + // `drop(dest[beg])`; self.block(vec![], TerminatorKind::Drop { - location: ret.index(beg), + location: dest.index(beg), target: BasicBlock::new(8), unwind: None, }, true); From 8a8f91b89effd9005c0ee536bcd8767304ac1363 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Jan 2018 17:36:19 +0530 Subject: [PATCH 8/8] Generalize tuple_like_shim's code to be useful for enums --- src/librustc_mir/shim.rs | 63 +++++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 58914097b43fb..42ffcc194ca8c 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -297,18 +297,22 @@ fn build_clone_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let mut builder = CloneShimBuilder::new(tcx, def_id, self_ty); let is_copy = !self_ty.moves_by_default(tcx, tcx.param_env(def_id), builder.span); + let dest = Place::Local(RETURN_PLACE); + let src = Place::Local(Local::new(1+0)).deref(); + match self_ty.sty { _ if is_copy => builder.copy_shim(), ty::TyArray(ty, len) => { let len = len.val.to_const_int().unwrap().to_u64().unwrap(); - builder.array_shim(ty, len) + builder.array_shim(dest, src, ty, len) } ty::TyClosure(def_id, substs) => { builder.tuple_like_shim( - &substs.upvar_tys(def_id, tcx).collect::>() + dest, src, + substs.upvar_tys(def_id, tcx) ) } - ty::TyTuple(tys, _) => builder.tuple_like_shim(&**tys), + ty::TyTuple(tys, _) => builder.tuple_like_shim(dest, src, tys.iter().cloned()), _ => { bug!("clone shim for `{:?}` which is not `Copy` and is not an aggregate", self_ty) } @@ -382,6 +386,14 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { }) } + /// Gives the index of an upcoming BasicBlock, with an offset. + /// offset=0 will give you the index of the next BasicBlock, + /// offset=1 will give the index of the next-to-next block, + /// offset=-1 will give you the index of the last-created block + fn block_index_offset(&mut self, offset: usize) -> BasicBlock { + BasicBlock::new(self.blocks.len() + offset) + } + fn make_statement(&self, kind: StatementKind<'tcx>) -> Statement<'tcx> { Statement { source_info: self.source_info(), @@ -502,11 +514,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { } } - fn array_shim(&mut self, ty: Ty<'tcx>, len: u64) { + fn array_shim(&mut self, dest: Place<'tcx>, src: Place<'tcx>, ty: Ty<'tcx>, len: u64) { let tcx = self.tcx; let span = self.span; - let src = Place::Local(Local::new(1+0)).deref(); - let dest = Place::Local(RETURN_PLACE); let beg = self.local_decls.push(temp_decl(Mutability::Mut, tcx.types.usize, span)); let end = self.make_place(Mutability::Not, tcx.types.usize); @@ -616,34 +626,39 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(vec![], TerminatorKind::Resume, true); } - fn tuple_like_shim(&mut self, tys: &[ty::Ty<'tcx>]) { - let rcvr = Place::Local(Local::new(1+0)).deref(); - - let mut previous_place = None; - let return_place = Place::Local(RETURN_PLACE); - for (i, ity) in tys.iter().enumerate() { + fn tuple_like_shim(&mut self, dest: Place<'tcx>, + src: Place<'tcx>, tys: I) + where I: Iterator> { + let mut previous_field = None; + for (i, ity) in tys.enumerate() { let field = Field::new(i); - let rcvr_field = rcvr.clone().field(field, *ity); + let src_field = src.clone().field(field, ity); + + let dest_field = dest.clone().field(field, ity); - let place = return_place.clone().field(field, *ity); + // #(2i + 1) is the cleanup block for the previous clone operation + let cleanup_block = self.block_index_offset(1); + // #(2i + 2) is the next cloning block + // (or the Return terminator if this is the last block) + let next_block = self.block_index_offset(2); // BB #(2i) - // `returns[i] = Clone::clone(&rcvr.i);` + // `dest.i = Clone::clone(&src.i);` // Goto #(2i + 2) if ok, #(2i + 1) if unwinding happens. self.make_clone_call( - place.clone(), - rcvr_field, - *ity, - BasicBlock::new(2 * i + 2), - BasicBlock::new(2 * i + 1), + dest_field.clone(), + src_field, + ity, + next_block, + cleanup_block, ); // BB #(2i + 1) (cleanup) - if let Some(previous_place) = previous_place.take() { + if let Some((previous_field, previous_cleanup)) = previous_field.take() { // Drop previous field and goto previous cleanup block. self.block(vec![], TerminatorKind::Drop { - location: previous_place, - target: BasicBlock::new(2 * i - 1), + location: previous_field, + target: previous_cleanup, unwind: None, }, true); } else { @@ -651,7 +666,7 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> { self.block(vec![], TerminatorKind::Resume, true); } - previous_place = Some(place); + previous_field = Some((dest_field, cleanup_block)); } self.block(vec![], TerminatorKind::Return, false);