Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid Operand::Copy with &mut T #72093

Merged
merged 3 commits into from
May 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/librustc_interface/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,7 @@ fn test_debugging_options_tracking_hash() {
untracked!(ui_testing, true);
untracked!(unpretty, Some("expanded".to_string()));
untracked!(unstable_options, true);
untracked!(validate_mir, true);
untracked!(verbose, true);

macro_rules! tracked {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ fn build_call_shim<'tcx>(

let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
Adjustment::Identity => Operand::Move(rcvr_place()),
Adjustment::Deref => Operand::Copy(tcx.mk_place_deref(rcvr_place())),
Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), // Can't copy `&mut`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a mir-opt test showing this

Adjustment::DerefMove => Operand::Move(tcx.mk_place_deref(rcvr_place())),
Adjustment::RefMut => {
// let rcvr = &mut rcvr;
Expand Down
19 changes: 12 additions & 7 deletions src/librustc_mir/transform/instcombine.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
//! Performs various peephole optimizations.

use crate::transform::{MirPass, MirSource};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::fx::FxHashMap;
use rustc_index::vec::Idx;
use rustc_middle::mir::visit::{MutVisitor, Visitor};
use rustc_middle::mir::{
Body, Constant, Local, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue,
Body, Constant, Local, Location, Mutability, Operand, Place, PlaceRef, ProjectionElem, Rvalue,
};
use rustc_middle::ty::{self, TyCtxt};
use std::mem;
Expand Down Expand Up @@ -39,7 +39,7 @@ impl<'tcx> MutVisitor<'tcx> for InstCombineVisitor<'tcx> {
}

fn visit_rvalue(&mut self, rvalue: &mut Rvalue<'tcx>, location: Location) {
if self.optimizations.and_stars.remove(&location) {
if let Some(mtbl) = self.optimizations.and_stars.remove(&location) {
debug!("replacing `&*`: {:?}", rvalue);
let new_place = match rvalue {
Rvalue::Ref(_, _, place) => {
Expand All @@ -57,7 +57,10 @@ impl<'tcx> MutVisitor<'tcx> for InstCombineVisitor<'tcx> {
}
_ => bug!("Detected `&*` but didn't find `&*`!"),
};
*rvalue = Rvalue::Use(Operand::Copy(new_place))
*rvalue = Rvalue::Use(match mtbl {
Mutability::Mut => Operand::Move(new_place),
Mutability::Not => Operand::Copy(new_place),
});
}

if let Some(constant) = self.optimizations.arrays_lengths.remove(&location) {
Expand Down Expand Up @@ -88,8 +91,10 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> {
if let PlaceRef { local, projection: &[ref proj_base @ .., ProjectionElem::Deref] } =
place.as_ref()
{
if Place::ty_from(local, proj_base, self.body, self.tcx).ty.is_region_ptr() {
self.optimizations.and_stars.insert(location);
// The dereferenced place must have type `&_`.
let ty = Place::ty_from(local, proj_base, self.body, self.tcx).ty;
if let ty::Ref(_, _, mtbl) = ty.kind {
self.optimizations.and_stars.insert(location, mtbl);
}
}
}
Expand All @@ -109,6 +114,6 @@ impl Visitor<'tcx> for OptimizationFinder<'b, 'tcx> {

#[derive(Default)]
struct OptimizationList<'tcx> {
and_stars: FxHashSet<Location>,
and_stars: FxHashMap<Location, Mutability>,
arrays_lengths: FxHashMap<Location, Constant<'tcx>>,
}
19 changes: 18 additions & 1 deletion src/librustc_mir/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub mod simplify_branches;
pub mod simplify_try;
pub mod uninhabited_enum_branching;
pub mod unreachable_prop;
pub mod validate;

pub(crate) fn provide(providers: &mut Providers<'_>) {
self::check_unsafety::provide(providers);
Expand Down Expand Up @@ -147,12 +148,18 @@ pub fn run_passes(
passes: &[&[&dyn MirPass<'tcx>]],
) {
let phase_index = mir_phase.phase_index();
let source = MirSource { instance, promoted };
let validate = tcx.sess.opts.debugging_opts.validate_mir;

if body.phase >= mir_phase {
return;
}

let source = MirSource { instance, promoted };
if validate {
validate::Validator { when: format!("input to phase {:?}", mir_phase) }
.run_pass(tcx, source, body);
}

let mut index = 0;
let mut run_pass = |pass: &dyn MirPass<'tcx>| {
let run_hooks = |body: &_, index, is_after| {
Expand All @@ -169,6 +176,11 @@ pub fn run_passes(
pass.run_pass(tcx, source, body);
run_hooks(body, index, true);

if validate {
validate::Validator { when: format!("after {} in phase {:?}", pass.name(), mir_phase) }
.run_pass(tcx, source, body);
}

index += 1;
};

Expand All @@ -179,6 +191,11 @@ pub fn run_passes(
}

body.phase = mir_phase;

if mir_phase == MirPhase::Optimized {
validate::Validator { when: format!("end of phase {:?}", mir_phase) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we run it here even without debug assertions? Isn't that expensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pass does very little work, so it's unlikely to be an issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pass does very little work

That is something I want to change.^^ #72796 does some non-trivial type folding.

.run_pass(tcx, source, body);
}
}

fn mir_const_qualif(tcx: TyCtxt<'_>, def_id: DefId) -> ConstQualifs {
Expand Down
80 changes: 80 additions & 0 deletions src/librustc_mir/transform/validate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
//! Validates the MIR to ensure that invariants are upheld.

use super::{MirPass, MirSource};
use rustc_middle::mir::visit::Visitor;
use rustc_middle::{
mir::{Body, Location, Operand, Rvalue, Statement, StatementKind},
ty::{ParamEnv, TyCtxt},
};
use rustc_span::{def_id::DefId, Span, DUMMY_SP};

pub struct Validator {
/// Describes at which point in the pipeline this validation is happening.
pub when: String,
}

impl<'tcx> MirPass<'tcx> for Validator {
fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) {
let def_id = source.def_id();
let param_env = tcx.param_env(def_id);
TypeChecker { when: &self.when, def_id, body, tcx, param_env }.visit_body(body);
}
}

struct TypeChecker<'a, 'tcx> {
when: &'a str,
def_id: DefId,
body: &'a Body<'tcx>,
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
}

impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
fn fail(&self, span: Span, msg: impl AsRef<str>) {
// We use `delay_span_bug` as we might see broken MIR when other errors have already
// occurred.
self.tcx.sess.diagnostic().delay_span_bug(
span,
&format!("broken MIR in {:?} ({}): {}", self.def_id, self.when, msg.as_ref()),
);
}
}

impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) {
// `Operand::Copy` is only supposed to be used with `Copy` types.
if let Operand::Copy(place) = operand {
let ty = place.ty(&self.body.local_decls, self.tcx).ty;

if !ty.is_copy_modulo_regions(self.tcx, self.param_env, DUMMY_SP) {
self.fail(
DUMMY_SP,
format!("`Operand::Copy` with non-`Copy` type {} at {:?}", ty, location),
);
}
}

self.super_operand(operand, location);
}

fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
// The sides of an assignment must not alias. Currently this just checks whether the places
// are identical.
if let StatementKind::Assign(box (dest, rvalue)) = &statement.kind {
match rvalue {
Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => {
if dest == src {
self.fail(
DUMMY_SP,
format!(
"encountered `Assign` statement with overlapping memory at {:?}",
location
),
);
}
}
_ => {}
}
}
}
}
2 changes: 2 additions & 0 deletions src/librustc_session/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"adds unstable command line options to rustc interface (default: no)"),
use_ctors_section: Option<bool> = (None, parse_opt_bool, [TRACKED],
"use legacy .ctors section for initializers rather than .init_array"),
validate_mir: bool = (false, parse_bool, [UNTRACKED],
"validate MIR after each transformation"),
verbose: bool = (false, parse_bool, [UNTRACKED],
"in general, enable more debug printouts (default: no)"),
verify_llvm_ir: bool = (false, parse_bool, [TRACKED],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +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
_3 = _4; // scope 1 at $SRC_DIR/libcore/convert/mod.rs:LL:COL
_3 = move _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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn b(_1: &mut std::boxed::Box<T>) -> &mut T {
_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
_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
_3 = move _5; // 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
Expand Down