From a9292d871c09bbfc2924dc6e7358fde3ba564d51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Fri, 15 Jan 2021 00:00:00 +0000 Subject: [PATCH] Remove disabled transformation from instcombine --- .../rustc_mir/src/transform/instcombine.rs | 145 +----------------- ...e_closure_borrows_arg.foo.Inline.after.mir | 2 +- src/test/mir-opt/inst_combine_deref.rs | 69 --------- 3 files changed, 4 insertions(+), 212 deletions(-) delete mode 100644 src/test/mir-opt/inst_combine_deref.rs diff --git a/compiler/rustc_mir/src/transform/instcombine.rs b/compiler/rustc_mir/src/transform/instcombine.rs index b0c70372b161..405f8ae36e85 100644 --- a/compiler/rustc_mir/src/transform/instcombine.rs +++ b/compiler/rustc_mir/src/transform/instcombine.rs @@ -4,14 +4,9 @@ use crate::transform::MirPass; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::Mutability; use rustc_index::vec::Idx; +use rustc_middle::mir::visit::{MutVisitor, Visitor}; use rustc_middle::mir::{ - visit::PlaceContext, - visit::{MutVisitor, Visitor}, - Statement, -}; -use rustc_middle::mir::{ - BinOp, Body, BorrowKind, Constant, Local, Location, Operand, Place, PlaceRef, ProjectionElem, - Rvalue, + BinOp, Body, Constant, Local, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue, }; use rustc_middle::ty::{self, TyCtxt}; use std::mem; @@ -90,38 +85,10 @@ impl<'tcx> MutVisitor<'tcx> for InstCombineVisitor<'tcx> { } } - if let Some(place) = self.optimizations.unneeded_deref.remove(&location) { - if self.should_combine(rvalue, location) { - debug!("unneeded_deref: replacing {:?} with {:?}", rvalue, place); - *rvalue = Rvalue::Use(Operand::Copy(place)); - } - } - // We do not call super_rvalue as we are not interested in any other parts of the tree } } -struct MutatingUseVisitor { - has_mutating_use: bool, - local_to_look_for: Local, -} - -impl MutatingUseVisitor { - fn has_mutating_use_in_stmt(local: Local, stmt: &Statement<'tcx>, location: Location) -> bool { - let mut _self = Self { has_mutating_use: false, local_to_look_for: local }; - _self.visit_statement(stmt, location); - _self.has_mutating_use - } -} - -impl<'tcx> Visitor<'tcx> for MutatingUseVisitor { - fn visit_local(&mut self, local: &Local, context: PlaceContext, _: Location) { - if *local == self.local_to_look_for { - self.has_mutating_use |= context.is_mutating_use(); - } - } -} - /// Finds optimization opportunities on the MIR. struct OptimizationFinder<'b, 'tcx> { body: &'b Body<'tcx>, @@ -134,103 +101,6 @@ impl OptimizationFinder<'b, 'tcx> { OptimizationFinder { body, tcx, optimizations: OptimizationList::default() } } - fn find_deref_of_address(&mut self, rvalue: &Rvalue<'tcx>, location: Location) -> Option<()> { - // FIXME(#78192): This optimization can result in unsoundness. - if !self.tcx.sess.opts.debugging_opts.unsound_mir_opts { - return None; - } - - // Look for the sequence - // - // _2 = &_1; - // ... - // _5 = (*_2); - // - // which we can replace the last statement with `_5 = _1;` to avoid the load of `_2`. - if let Rvalue::Use(op) = rvalue { - let local_being_derefed = match op.place()?.as_ref() { - PlaceRef { local, projection: [ProjectionElem::Deref] } => Some(local), - _ => None, - }?; - - let mut dead_locals_seen = vec![]; - - let stmt_index = location.statement_index; - // Look behind for statement that assigns the local from a address of operator. - // 6 is chosen as a heuristic determined by seeing the number of times - // the optimization kicked in compiling rust std. - let lower_index = stmt_index.saturating_sub(6); - let statements_to_look_in = self.body.basic_blocks()[location.block].statements - [lower_index..stmt_index] - .iter() - .rev(); - for stmt in statements_to_look_in { - match &stmt.kind { - // Exhaustive match on statements to detect conditions that warrant we bail out of the optimization. - rustc_middle::mir::StatementKind::Assign(box (l, r)) - if l.local == local_being_derefed => - { - match r { - // Looking for immutable reference e.g _local_being_deref = &_1; - Rvalue::Ref( - _, - // Only apply the optimization if it is an immutable borrow. - BorrowKind::Shared, - place_taken_address_of, - ) => { - // Make sure that the place has not been marked dead - if dead_locals_seen.contains(&place_taken_address_of.local) { - return None; - } - - self.optimizations - .unneeded_deref - .insert(location, *place_taken_address_of); - return Some(()); - } - - // We found an assignment of `local_being_deref` that is not an immutable ref, e.g the following sequence - // _2 = &_1; - // _3 = &5 - // _2 = _3; <-- this means it is no longer valid to replace the last statement with `_5 = _1;` - // _5 = (*_2); - _ => return None, - } - } - - // Inline asm can do anything, so bail out of the optimization. - rustc_middle::mir::StatementKind::LlvmInlineAsm(_) => return None, - - // Remember `StorageDead`s, as the local being marked dead could be the - // place RHS we are looking for, in which case we need to abort to avoid UB - // using an uninitialized place - rustc_middle::mir::StatementKind::StorageDead(dead) => { - dead_locals_seen.push(*dead) - } - - // Check that `local_being_deref` is not being used in a mutating way which can cause misoptimization. - rustc_middle::mir::StatementKind::Assign(box (_, _)) - | rustc_middle::mir::StatementKind::Coverage(_) - | rustc_middle::mir::StatementKind::Nop - | rustc_middle::mir::StatementKind::FakeRead(_, _) - | rustc_middle::mir::StatementKind::StorageLive(_) - | rustc_middle::mir::StatementKind::Retag(_, _) - | rustc_middle::mir::StatementKind::AscribeUserType(_, _) - | rustc_middle::mir::StatementKind::SetDiscriminant { .. } => { - if MutatingUseVisitor::has_mutating_use_in_stmt( - local_being_derefed, - stmt, - location, - ) { - return None; - } - } - } - } - } - Some(()) - } - fn find_unneeded_equality_comparison(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { // find Ne(_place, false) or Ne(false, _place) // or Eq(_place, true) or Eq(true, _place) @@ -295,8 +165,6 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> { } } - let _ = self.find_deref_of_address(rvalue, location); - self.find_unneeded_equality_comparison(rvalue, location); // We do not call super_rvalue as we are not interested in any other parts of the tree @@ -308,22 +176,15 @@ struct OptimizationList<'tcx> { and_stars: FxHashSet, arrays_lengths: FxHashMap>, unneeded_equality_comparison: FxHashMap>, - unneeded_deref: FxHashMap>, } impl<'tcx> OptimizationList<'tcx> { fn is_empty(&self) -> bool { match self { - OptimizationList { - and_stars, - arrays_lengths, - unneeded_equality_comparison, - unneeded_deref, - } => { + OptimizationList { and_stars, arrays_lengths, unneeded_equality_comparison } => { and_stars.is_empty() && arrays_lengths.is_empty() && unneeded_equality_comparison.is_empty() - && unneeded_deref.is_empty() } } } diff --git a/src/test/mir-opt/inline/inline_closure_borrows_arg.foo.Inline.after.mir b/src/test/mir-opt/inline/inline_closure_borrows_arg.foo.Inline.after.mir index db504b416fe1..4bda9ae383c2 100644 --- a/src/test/mir-opt/inline/inline_closure_borrows_arg.foo.Inline.after.mir +++ b/src/test/mir-opt/inline/inline_closure_borrows_arg.foo.Inline.after.mir @@ -40,7 +40,7 @@ fn foo(_1: T, _2: &i32) -> i32 { _9 = move (_5.1: &i32); // scope 1 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 StorageLive(_10); // scope 2 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 _10 = _8; // scope 2 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 - _0 = (*_8); // scope 3 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 + _0 = (*_10); // scope 3 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 StorageDead(_10); // scope 2 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 StorageDead(_9); // scope 1 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 StorageDead(_8); // scope 1 at $DIR/inline-closure-borrows-arg.rs:16:5: 16:12 diff --git a/src/test/mir-opt/inst_combine_deref.rs b/src/test/mir-opt/inst_combine_deref.rs deleted file mode 100644 index 78361c336607..000000000000 --- a/src/test/mir-opt/inst_combine_deref.rs +++ /dev/null @@ -1,69 +0,0 @@ -// compile-flags: -O -Zunsound-mir-opts -// EMIT_MIR inst_combine_deref.simple_opt.InstCombine.diff -fn simple_opt() -> u64 { - let x = 5; - let y = &x; - let z = *y; - z -} - -// EMIT_MIR inst_combine_deref.deep_opt.InstCombine.diff -fn deep_opt() -> (u64, u64, u64) { - let x1 = 1; - let x2 = 2; - let x3 = 3; - let y1 = &x1; - let y2 = &x2; - let y3 = &x3; - let z1 = *y1; - let z2 = *y2; - let z3 = *y3; - (z1, z2, z3) -} - -struct S { - a: u64, - b: u64, -} - -// EMIT_MIR inst_combine_deref.opt_struct.InstCombine.diff -fn opt_struct(s: S) -> u64 { - let a = &s.a; - let b = &s.b; - let x = *a; - *b + x -} - -// EMIT_MIR inst_combine_deref.dont_opt.InstCombine.diff -// do not optimize a sequence looking like this: -// _1 = &_2; -// _1 = _3; -// _4 = *_1; -// as the _1 = _3 assignment makes it not legal to replace the last statement with _4 = _2 -fn dont_opt() -> u64 { - let y = 5; - let _ref = &y; - let x = 5; - let mut _1 = &x; - _1 = _ref; - let _4 = *_1; - 0 -} - -// EMIT_MIR inst_combine_deref.do_not_miscompile.InstCombine.diff -fn do_not_miscompile() { - let x = 42; - let a = 99; - let mut y = &x; - let z = &mut y; - *z = &a; - assert!(*y == 99); -} - -fn main() { - simple_opt(); - deep_opt(); - opt_struct(S { a: 0, b: 1 }); - dont_opt(); - do_not_miscompile(); -}