From ae5553d7b04d8ece340756f378b82d5e0bbb0ea2 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Sun, 5 Nov 2017 22:08:30 +0900 Subject: [PATCH 1/2] Fix MIR CopyPropagation errneously propagating assignments to function arguments --- src/librustc_mir/transform/copy_prop.rs | 7 ++ src/test/mir-opt/copy_propagation_arg.rs | 115 +++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 src/test/mir-opt/copy_propagation_arg.rs diff --git a/src/librustc_mir/transform/copy_prop.rs b/src/librustc_mir/transform/copy_prop.rs index ac8ebd306d321..3f766629bae5c 100644 --- a/src/librustc_mir/transform/copy_prop.rs +++ b/src/librustc_mir/transform/copy_prop.rs @@ -99,6 +99,13 @@ impl MirPass for CopyPropagation { dest_local); continue } + // Conservatively gives up if the dest is an argument, + // because there may be uses of the original argument value. + if mir.local_kind(dest_local) == LocalKind::Arg { + debug!(" Can't copy-propagate local: dest {:?} (argument)", + dest_local); + continue; + } let dest_lvalue_def = dest_use_info.defs_and_uses.iter().filter(|lvalue_def| { lvalue_def.context.is_mutating_use() && !lvalue_def.context.is_drop() }).next().unwrap(); diff --git a/src/test/mir-opt/copy_propagation_arg.rs b/src/test/mir-opt/copy_propagation_arg.rs new file mode 100644 index 0000000000000..8303407d2e20c --- /dev/null +++ b/src/test/mir-opt/copy_propagation_arg.rs @@ -0,0 +1,115 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Check that CopyPropagation does not propagate an assignment to a function argument +// (doing so can break usages of the original argument value) + +fn dummy(x: u8) -> u8 { + x +} + +fn foo(mut x: u8) { + // calling `dummy` to make an use of `x` that copyprop cannot eliminate + x = dummy(x); // this will assign a local to `x` +} + +fn bar(mut x: u8) { + dummy(x); + x = 5; +} + +fn baz(mut x: i32) { + // self-assignment to a function argument should be eliminated + x = x; +} + +fn main() { + // Make sure the function actually gets instantiated. + foo(0); + bar(0); +} + +// END RUST SOURCE +// START rustc.foo.CopyPropagation.before.mir +// bb0: { +// StorageLive(_2); +// StorageLive(_3); +// _3 = _1; +// _2 = const dummy(_3) -> bb1; +// } +// bb1: { +// StorageDead(_3); +// _1 = _2; +// StorageDead(_2); +// _0 = (); +// return; +// } +// END rustc.foo.CopyPropagation.before.mir +// START rustc.foo.CopyPropagation.after.mir +// bb0: { +// StorageLive(_2); +// nop; +// nop; +// _2 = const dummy(_1) -> bb1; +// } +// bb1: { +// nop; +// _1 = _2; +// StorageDead(_2); +// _0 = (); +// return; +// } +// END rustc.foo.CopyPropagation.after.mir +// START rustc.bar.CopyPropagation.before.mir +// bb0: { +// StorageLive(_3); +// _3 = _1; +// _2 = const dummy(_3) -> bb1; +// } +// bb1: { +// StorageDead(_3); +// _1 = const 5u8; +// _0 = (); +// return; +// } +// END rustc.bar.CopyPropagation.before.mir +// START rustc.bar.CopyPropagation.after.mir +// bb0: { +// nop; +// nop; +// _2 = const dummy(_1) -> bb1; +// } +// bb1: { +// nop; +// _1 = const 5u8; +// _0 = (); +// return; +// } +// END rustc.bar.CopyPropagation.after.mir +// START rustc.baz.CopyPropagation.before.mir +// bb0: { +// StorageLive(_2); +// _2 = _1; +// _1 = _2; +// StorageDead(_2); +// _0 = (); +// return; +// } +// END rustc.baz.CopyPropagation.before.mir +// START rustc.baz.CopyPropagation.after.mir +// bb0: { +// nop; +// nop; +// nop; +// nop; +// _0 = (); +// return; +// } +// END rustc.baz.CopyPropagation.after.mir \ No newline at end of file From 114252410d72b63bda4eefa62df77727d1f1cc41 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Mon, 6 Nov 2017 12:35:43 +0900 Subject: [PATCH 2/2] Separately eliminate self-assignments --- src/librustc_mir/transform/copy_prop.rs | 43 +++++++++++++++++++++--- src/librustc_mir/util/def_use.rs | 22 +++++++++++- src/test/mir-opt/copy_propagation_arg.rs | 3 +- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/transform/copy_prop.rs b/src/librustc_mir/transform/copy_prop.rs index 3f766629bae5c..a730fc30615b4 100644 --- a/src/librustc_mir/transform/copy_prop.rs +++ b/src/librustc_mir/transform/copy_prop.rs @@ -69,10 +69,14 @@ impl MirPass for CopyPropagation { return; } + let mut def_use_analysis = DefUseAnalysis::new(mir); loop { - let mut def_use_analysis = DefUseAnalysis::new(mir); def_use_analysis.analyze(mir); + if eliminate_self_assignments(mir, &def_use_analysis) { + def_use_analysis.analyze(mir); + } + let mut changed = false; for dest_local in mir.local_decls.indices() { debug!("Considering destination local: {:?}", dest_local); @@ -106,9 +110,7 @@ impl MirPass for CopyPropagation { dest_local); continue; } - let dest_lvalue_def = dest_use_info.defs_and_uses.iter().filter(|lvalue_def| { - lvalue_def.context.is_mutating_use() && !lvalue_def.context.is_drop() - }).next().unwrap(); + let dest_lvalue_def = dest_use_info.defs_not_including_drop().next().unwrap(); location = dest_lvalue_def.location; let basic_block = &mir[location.block]; @@ -158,6 +160,39 @@ impl MirPass for CopyPropagation { } } +fn eliminate_self_assignments<'tcx>( + mir: &mut Mir<'tcx>, + def_use_analysis: &DefUseAnalysis<'tcx>, +) -> bool { + let mut changed = false; + + for dest_local in mir.local_decls.indices() { + let dest_use_info = def_use_analysis.local_info(dest_local); + + for def in dest_use_info.defs_not_including_drop() { + let location = def.location; + if let Some(stmt) = mir[location.block].statements.get(location.statement_index) { + match stmt.kind { + StatementKind::Assign( + Lvalue::Local(local), + Rvalue::Use(Operand::Consume(Lvalue::Local(src_local))), + ) if local == dest_local && dest_local == src_local => {} + _ => { + continue; + } + } + } else { + continue; + } + debug!("Deleting a self-assignment for {:?}", dest_local); + mir.make_statement_nop(location); + changed = true; + } + } + + changed +} + enum Action<'tcx> { PropagateLocalCopy(Local), PropagateConstant(Constant<'tcx>), diff --git a/src/librustc_mir/util/def_use.rs b/src/librustc_mir/util/def_use.rs index bd9fb4bc3cc5f..9ada8f2bebf70 100644 --- a/src/librustc_mir/util/def_use.rs +++ b/src/librustc_mir/util/def_use.rs @@ -15,6 +15,8 @@ use rustc::mir::visit::{LvalueContext, MutVisitor, Visitor}; use rustc_data_structures::indexed_vec::IndexVec; use std::marker::PhantomData; use std::mem; +use std::slice; +use std::iter; pub struct DefUseAnalysis<'tcx> { info: IndexVec>, @@ -39,6 +41,8 @@ impl<'tcx> DefUseAnalysis<'tcx> { } pub fn analyze(&mut self, mir: &Mir<'tcx>) { + self.clear(); + let mut finder = DefUseFinder { info: mem::replace(&mut self.info, IndexVec::new()), }; @@ -46,6 +50,12 @@ impl<'tcx> DefUseAnalysis<'tcx> { self.info = finder.info } + fn clear(&mut self) { + for info in &mut self.info { + info.clear(); + } + } + pub fn local_info(&self, local: Local) -> &Info<'tcx> { &self.info[local] } @@ -93,14 +103,24 @@ impl<'tcx> Info<'tcx> { } } + fn clear(&mut self) { + self.defs_and_uses.clear(); + } + pub fn def_count(&self) -> usize { self.defs_and_uses.iter().filter(|lvalue_use| lvalue_use.context.is_mutating_use()).count() } pub fn def_count_not_including_drop(&self) -> usize { + self.defs_not_including_drop().count() + } + + pub fn defs_not_including_drop( + &self, + ) -> iter::Filter>, fn(&&Use<'tcx>) -> bool> { self.defs_and_uses.iter().filter(|lvalue_use| { lvalue_use.context.is_mutating_use() && !lvalue_use.context.is_drop() - }).count() + }) } pub fn use_count(&self) -> usize { diff --git a/src/test/mir-opt/copy_propagation_arg.rs b/src/test/mir-opt/copy_propagation_arg.rs index 8303407d2e20c..ae30b5fae88f8 100644 --- a/src/test/mir-opt/copy_propagation_arg.rs +++ b/src/test/mir-opt/copy_propagation_arg.rs @@ -34,6 +34,7 @@ fn main() { // Make sure the function actually gets instantiated. foo(0); bar(0); + baz(0); } // END RUST SOURCE @@ -112,4 +113,4 @@ fn main() { // _0 = (); // return; // } -// END rustc.baz.CopyPropagation.after.mir \ No newline at end of file +// END rustc.baz.CopyPropagation.after.mir