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

rustc_mir_transform cleanups 3 #130175

Merged
merged 8 commits into from
Sep 10, 2024
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
6 changes: 0 additions & 6 deletions compiler/rustc_mir_transform/src/add_call_guards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ pub(super) use self::AddCallGuards::*;

impl<'tcx> crate::MirPass<'tcx> for AddCallGuards {
fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
self.add_call_guards(body);
}
}

impl AddCallGuards {
pub(super) fn add_call_guards(&self, body: &mut Body<'_>) {
let mut pred_count: IndexVec<_, _> =
body.basic_blocks.predecessors().iter().map(|ps| ps.len()).collect();
pred_count[START_BLOCK] += 1;
Expand Down
45 changes: 22 additions & 23 deletions compiler/rustc_mir_transform/src/add_moves_for_packed_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,35 +40,34 @@ pub(super) struct AddMovesForPackedDrops;
impl<'tcx> crate::MirPass<'tcx> for AddMovesForPackedDrops {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
debug!("add_moves_for_packed_drops({:?} @ {:?})", body.source, body.span);
add_moves_for_packed_drops(tcx, body);
}
}

fn add_moves_for_packed_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let patch = add_moves_for_packed_drops_patch(tcx, body);
patch.apply(body);
}

fn add_moves_for_packed_drops_patch<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> MirPatch<'tcx> {
let def_id = body.source.def_id();
let mut patch = MirPatch::new(body);
let param_env = tcx.param_env(def_id);
let def_id = body.source.def_id();
let mut patch = MirPatch::new(body);
let param_env = tcx.param_env(def_id);

for (bb, data) in body.basic_blocks.iter_enumerated() {
let loc = Location { block: bb, statement_index: data.statements.len() };
let terminator = data.terminator();
for (bb, data) in body.basic_blocks.iter_enumerated() {
let loc = Location { block: bb, statement_index: data.statements.len() };
let terminator = data.terminator();

match terminator.kind {
TerminatorKind::Drop { place, .. }
if util::is_disaligned(tcx, body, param_env, place) =>
{
add_move_for_packed_drop(tcx, body, &mut patch, terminator, loc, data.is_cleanup);
match terminator.kind {
TerminatorKind::Drop { place, .. }
if util::is_disaligned(tcx, body, param_env, place) =>
{
add_move_for_packed_drop(
tcx,
body,
&mut patch,
terminator,
loc,
data.is_cleanup,
);
}
_ => {}
}
_ => {}
}
}

patch
patch.apply(body);
}
}

fn add_move_for_packed_drop<'tcx>(
Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_mir_transform/src/add_retag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ impl<'tcx> crate::MirPass<'tcx> for AddRetag {
let basic_blocks = body.basic_blocks.as_mut();
let local_decls = &body.local_decls;
let needs_retag = |place: &Place<'tcx>| {
!place.is_indirect_first_projection() // we're not really interested in stores to "outside" locations, they are hard to keep track of anyway
// We're not really interested in stores to "outside" locations, they are hard to keep
// track of anyway.
!place.is_indirect_first_projection()
&& may_contain_reference(place.ty(&*local_decls, tcx).ty, /*depth*/ 3, tcx)
&& !local_decls[place.local].is_deref_temp()
};
Expand Down Expand Up @@ -129,9 +131,9 @@ impl<'tcx> crate::MirPass<'tcx> for AddRetag {
StatementKind::Assign(box (ref place, ref rvalue)) => {
let add_retag = match rvalue {
// Ptr-creating operations already do their own internal retagging, no
// need to also add a retag statement.
// *Except* if we are deref'ing a Box, because those get desugared to directly working
// with the inner raw pointer! That's relevant for `RawPtr` as Miri otherwise makes it
// need to also add a retag statement. *Except* if we are deref'ing a
// Box, because those get desugared to directly working with the inner
// raw pointer! That's relevant for `RawPtr` as Miri otherwise makes it
Comment on lines +134 to +136
Copy link
Member

Choose a reason for hiding this comment

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

My kingdom for a code review tool that can ignore this diff

// a NOP when the original pointer is already raw.
Rvalue::RawPtr(_mutbl, place) => {
// Using `is_box_global` here is a bit sketchy: if this code is
Expand Down
18 changes: 7 additions & 11 deletions compiler/rustc_mir_transform/src/add_subtyping_projections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,14 @@ impl<'a, 'tcx> MutVisitor<'tcx> for SubTypeChecker<'a, 'tcx> {
// // gets transformed to
// let temp: rval_ty = rval;
// let place: place_ty = temp as place_ty;
fn subtype_finder<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let patch = MirPatch::new(body);
let mut checker = SubTypeChecker { tcx, patcher: patch, local_decls: &body.local_decls };

for (bb, data) in body.basic_blocks.as_mut_preserves_cfg().iter_enumerated_mut() {
checker.visit_basic_block_data(bb, data);
}
checker.patcher.apply(body);
}

impl<'tcx> crate::MirPass<'tcx> for Subtyper {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
subtype_finder(tcx, body);
let patch = MirPatch::new(body);
let mut checker = SubTypeChecker { tcx, patcher: patch, local_decls: &body.local_decls };

for (bb, data) in body.basic_blocks.as_mut_preserves_cfg().iter_enumerated_mut() {
checker.visit_basic_block_data(bb, data);
}
checker.patcher.apply(body);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ impl<'tcx> Visitor<'tcx> for ConstMutationChecker<'_, 'tcx> {
self.super_statement(stmt, loc);
self.target_local = None;
}

fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, loc: Location) {
if let Rvalue::Ref(_, BorrowKind::Mut { .. }, place) = rvalue {
let local = place.local;
Expand Down
48 changes: 23 additions & 25 deletions compiler/rustc_mir_transform/src/copy_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,37 +27,34 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp {
#[instrument(level = "trace", skip(self, tcx, body))]
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
debug!(def_id = ?body.source.def_id());
propagate_ssa(tcx, body);
}
}

fn propagate_ssa<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id());
let ssa = SsaLocals::new(tcx, body, param_env);
let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id());
let ssa = SsaLocals::new(tcx, body, param_env);

let fully_moved = fully_moved_locals(&ssa, body);
debug!(?fully_moved);
let fully_moved = fully_moved_locals(&ssa, body);
debug!(?fully_moved);

let mut storage_to_remove = BitSet::new_empty(fully_moved.domain_size());
for (local, &head) in ssa.copy_classes().iter_enumerated() {
if local != head {
storage_to_remove.insert(head);
let mut storage_to_remove = BitSet::new_empty(fully_moved.domain_size());
for (local, &head) in ssa.copy_classes().iter_enumerated() {
if local != head {
storage_to_remove.insert(head);
}
}
}

let any_replacement = ssa.copy_classes().iter_enumerated().any(|(l, &h)| l != h);
let any_replacement = ssa.copy_classes().iter_enumerated().any(|(l, &h)| l != h);

Replacer {
tcx,
copy_classes: ssa.copy_classes(),
fully_moved,
borrowed_locals: ssa.borrowed_locals(),
storage_to_remove,
}
.visit_body_preserves_cfg(body);
Replacer {
tcx,
copy_classes: ssa.copy_classes(),
fully_moved,
borrowed_locals: ssa.borrowed_locals(),
storage_to_remove,
}
.visit_body_preserves_cfg(body);

if any_replacement {
crate::simplify::remove_unused_definitions(body);
if any_replacement {
crate::simplify::remove_unused_definitions(body);
}
}
}

Expand Down Expand Up @@ -140,7 +137,8 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> {

fn visit_operand(&mut self, operand: &mut Operand<'tcx>, loc: Location) {
if let Operand::Move(place) = *operand
// A move out of a projection of a copy is equivalent to a copy of the original projection.
// A move out of a projection of a copy is equivalent to a copy of the original
// projection.
Comment on lines +140 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: These single-word reflows are a bit awkward.

For single-line comments that don't quite fit in 100 columns, I'd consider breaking them even earlier (closer to 80 columns) just so that the second line is a bit longer.

(But if you're using automation to do this then maybe it's not worth the extra effort. 🤷‍♂️)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reflowed them all to width 100, because that's the closest I could get to the original while observing the "don't exceed 100 chars" rule. I definitely don't want to be thinking about different widths for different comments depending on their contents. One word on a line by itself is fine.

&& !place.is_indirect_first_projection()
&& !self.fully_moved.contains(place.local)
{
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,8 @@ fn inject_mcdc_statements<'tcx>(
basic_coverage_blocks: &CoverageGraph,
extracted_mappings: &ExtractedMappings,
) {
// Inject test vector update first because `inject_statement` always insert new statement at head.
// Inject test vector update first because `inject_statement` always insert new statement at
// head.
for &mappings::MCDCDecision {
span: _,
ref end_bcbs,
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ fn try_write_constant<'tcx>(
ty::FnDef(..) => {}

// Those are scalars, must be handled above.
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char => throw_machine_stop_str!("primitive type with provenance"),
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char =>
throw_machine_stop_str!("primitive type with provenance"),

ty::Tuple(elem_tys) => {
for (i, elem) in elem_tys.iter().enumerate() {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/deduce_param_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ impl<'tcx> Visitor<'tcx> for DeduceReadOnly {
}
PlaceContext::NonMutatingUse(NonMutatingUseContext::RawBorrow) => {
// Whether mutating though a `&raw const` is allowed is still undecided, so we
// disable any sketchy `readonly` optimizations for now.
// But we only need to do this if the pointer would point into the argument.
// IOW: for indirect places, like `&raw (*local).field`, this surely cannot mutate `local`.
// disable any sketchy `readonly` optimizations for now. But we only need to do
// this if the pointer would point into the argument. IOW: for indirect places,
// like `&raw (*local).field`, this surely cannot mutate `local`.
!place.is_indirect()
}
PlaceContext::NonMutatingUse(..) | PlaceContext::NonUse(..) => {
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_mir_transform/src/deduplicate_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ fn find_duplicates(body: &Body<'_>) -> FxHashMap<BasicBlock, BasicBlock> {
// For example, if bb1, bb2 and bb3 are duplicates, we will first insert bb3 in same_hashes.
// Then we will see that bb2 is a duplicate of bb3,
// and insert bb2 with the replacement bb3 in the duplicates list.
// When we see bb1, we see that it is a duplicate of bb3, and therefore insert it in the duplicates list
// with replacement bb3.
// When we see bb1, we see that it is a duplicate of bb3, and therefore insert it in the
// duplicates list with replacement bb3.
// When the duplicates are removed, we will end up with only bb3.
for (bb, bbd) in body.basic_blocks.iter_enumerated().rev().filter(|(_, bbd)| !bbd.is_cleanup) {
// Basic blocks can get really big, so to avoid checking for duplicates in basic blocks
Expand Down Expand Up @@ -105,7 +105,8 @@ struct BasicBlockHashable<'tcx, 'a> {
impl Hash for BasicBlockHashable<'_, '_> {
fn hash<H: Hasher>(&self, state: &mut H) {
hash_statements(state, self.basic_block_data.statements.iter());
// Note that since we only hash the kind, we lose span information if we deduplicate the blocks
// Note that since we only hash the kind, we lose span information if we deduplicate the
// blocks.
self.basic_block_data.terminator().kind.hash(state);
}
}
Expand Down
14 changes: 7 additions & 7 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl<'tcx> crate::MirPass<'tcx> for DestinationPropagation {
}
round_count += 1;

apply_merges(body, tcx, &merges, &merged_locals);
apply_merges(body, tcx, merges, merged_locals);
}

trace!(round_count);
Expand Down Expand Up @@ -281,20 +281,20 @@ struct Candidates {
fn apply_merges<'tcx>(
body: &mut Body<'tcx>,
tcx: TyCtxt<'tcx>,
merges: &FxIndexMap<Local, Local>,
merged_locals: &BitSet<Local>,
merges: FxIndexMap<Local, Local>,
merged_locals: BitSet<Local>,
) {
let mut merger = Merger { tcx, merges, merged_locals };
merger.visit_body_preserves_cfg(body);
}

struct Merger<'a, 'tcx> {
struct Merger<'tcx> {
tcx: TyCtxt<'tcx>,
merges: &'a FxIndexMap<Local, Local>,
merged_locals: &'a BitSet<Local>,
merges: FxIndexMap<Local, Local>,
merged_locals: BitSet<Local>,
}

impl<'a, 'tcx> MutVisitor<'tcx> for Merger<'a, 'tcx> {
impl<'tcx> MutVisitor<'tcx> for Merger<'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/early_otherwise_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ fn evaluate_candidate<'tcx>(
// };
// ```
//
// Hoisting the `discriminant(Q)` out of the `A` arm causes us to compute the discriminant of an
// invalid value, which is UB.
// Hoisting the `discriminant(Q)` out of the `A` arm causes us to compute the discriminant
// of an invalid value, which is UB.
// In order to fix this, **we would either need to show that the discriminant computation of
// `place` is computed in all branches**.
// FIXME(#95162) For the moment, we adopt a conservative approach and
Expand Down
16 changes: 8 additions & 8 deletions compiler/rustc_mir_transform/src/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use tracing::{debug, instrument};
use crate::deref_separator::deref_finder;

/// During MIR building, Drop terminators are inserted in every place where a drop may occur.
/// However, in this phase, the presence of these terminators does not guarantee that a destructor will run,
/// as the target of the drop may be uninitialized.
/// However, in this phase, the presence of these terminators does not guarantee that a destructor
/// will run, as the target of the drop may be uninitialized.
/// In general, the compiler cannot determine at compile time whether a destructor will run or not.
///
/// At a high level, this pass refines Drop to only run the destructor if the
Expand All @@ -30,10 +30,10 @@ use crate::deref_separator::deref_finder;
/// Once this is complete, Drop terminators in the MIR correspond to a call to the "drop glue" or
/// "drop shim" for the type of the dropped place.
///
/// This pass relies on dropped places having an associated move path, which is then used to determine
/// the initialization status of the place and its descendants.
/// It's worth noting that a MIR containing a Drop without an associated move path is probably ill formed,
/// as it would allow running a destructor on a place behind a reference:
/// This pass relies on dropped places having an associated move path, which is then used to
/// determine the initialization status of the place and its descendants.
/// It's worth noting that a MIR containing a Drop without an associated move path is probably ill
/// formed, as it would allow running a destructor on a place behind a reference:
///
/// ```text
/// fn drop_term<T>(t: &mut T) {
Expand Down Expand Up @@ -377,8 +377,8 @@ impl<'a, 'tcx> ElaborateDropsCtxt<'a, 'tcx> {
);
}
// A drop and replace behind a pointer/array/whatever.
// The borrow checker requires that these locations are initialized before the assignment,
// so we just leave an unconditional drop.
// The borrow checker requires that these locations are initialized before the
// assignment, so we just leave an unconditional drop.
assert!(!data.is_cleanup);
}
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_mir_transform/src/ffi_unwind_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ fn has_ffi_unwind_calls(tcx: TyCtxt<'_>, local_def_id: LocalDefId) -> bool {
let fn_def_id = match ty.kind() {
ty::FnPtr(..) => None,
&ty::FnDef(def_id, _) => {
// Rust calls cannot themselves create foreign unwinds (even if they use a non-Rust ABI).
// So the leak of the foreign unwind into Rust can only be elsewhere, not here.
// Rust calls cannot themselves create foreign unwinds (even if they use a non-Rust
// ABI). So the leak of the foreign unwind into Rust can only be elsewhere, not
// here.
if !tcx.is_foreign_item(def_id) {
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/function_item_references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ impl<'tcx> FunctionItemRefChecker<'_, 'tcx> {
{
let mut span = self.nth_arg_span(args, arg_num);
if span.from_expansion() {
// The operand's ctxt wouldn't display the lint since it's inside a macro so
// we have to use the callsite's ctxt.
// The operand's ctxt wouldn't display the lint since it's
// inside a macro so we have to use the callsite's ctxt.
let callsite_ctxt = span.source_callsite().ctxt();
span = span.with_ctxt(callsite_ctxt);
}
Expand Down
Loading
Loading