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, round 2 #129929

Merged
merged 10 commits into from
Sep 9, 2024
43 changes: 15 additions & 28 deletions compiler/rustc_mir_transform/src/abort_unwinding_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_target::spec::PanicStrategy;
/// This forces all unwinds, in panic=abort mode happening in foreign code, to
/// trigger a process abort.
#[derive(PartialEq)]
pub struct AbortUnwindingCalls;
pub(super) struct AbortUnwindingCalls;

impl<'tcx> crate::MirPass<'tcx> for AbortUnwindingCalls {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Expand Down Expand Up @@ -50,9 +50,7 @@ impl<'tcx> crate::MirPass<'tcx> for AbortUnwindingCalls {
// with a function call, and whose function we're calling may unwind.
// This will filter to functions with `extern "C-unwind"` ABIs, for
// example.
let mut calls_to_terminate = Vec::new();
let mut cleanups_to_remove = Vec::new();
for (id, block) in body.basic_blocks.iter_enumerated() {
for block in body.basic_blocks.as_mut() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes behaviour: basic_block.as_mut() clears the cfg caches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I will change it to as_mut_preserves_cfg.

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 have switched to as_mut_preserves_cfg, but I'm not certain if it's correct.

When you say "changes behaviour", clearing the caches might hurt perf, but won't have any visibile functional effect, right? But switching to as_mut_preserves_cfg could have a visible effect. I can just remove this commit if necessary.

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 could just go back to as_mut() and let the cache be cleared, and measure the perf effect. I suspect it will be negligible.

Copy link
Contributor

@cjgillot cjgillot Sep 6, 2024

Choose a reason for hiding this comment

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

Using this is actually unsound, as you are modifying the CFG.
You need to call body.basic_blocks.invalidate_cfg_cache() explicitly if you use this.
3 options:

  • keep as-is;
  • use as_mut() and potentially clear the cache -> probably not an issue, as this pass runs just after ElaborateDrops which changes the CFG a lot, so we may have nothing in cache;
  • use as_mut_preserves_cfg() and call invalidate_cfg_cache() explicitly -> I'm not super fond, as that's a footgun.

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 measured and found that the cache is always totally empty when this code path is hit. So I will convert back to as_mut and I think that should be good enough.

if block.is_cleanup {
continue;
}
Expand All @@ -61,7 +59,7 @@ impl<'tcx> crate::MirPass<'tcx> for AbortUnwindingCalls {

let call_can_unwind = match &terminator.kind {
TerminatorKind::Call { func, .. } => {
let ty = func.ty(body, tcx);
let ty = func.ty(&body.local_decls, tcx);
let sig = ty.fn_sig(tcx);
let fn_def_id = match ty.kind() {
ty::FnPtr(..) => None,
Expand All @@ -86,33 +84,22 @@ impl<'tcx> crate::MirPass<'tcx> for AbortUnwindingCalls {
_ => continue,
};

// If this function call can't unwind, then there's no need for it
// to have a landing pad. This means that we can remove any cleanup
// registered for it.
if !call_can_unwind {
cleanups_to_remove.push(id);
continue;
}

// Otherwise if this function can unwind, then if the outer function
// can also unwind there's nothing to do. If the outer function
// can't unwind, however, we need to change the landing pad for this
// function call to one that aborts.
if !body_can_unwind {
calls_to_terminate.push(id);
// If this function call can't unwind, then there's no need for it
// to have a landing pad. This means that we can remove any cleanup
// registered for it.
let cleanup = block.terminator_mut().unwind_mut().unwrap();
*cleanup = UnwindAction::Unreachable;
} else if !body_can_unwind {
// Otherwise if this function can unwind, then if the outer function
// can also unwind there's nothing to do. If the outer function
// can't unwind, however, we need to change the landing pad for this
// function call to one that aborts.
let cleanup = block.terminator_mut().unwind_mut().unwrap();
*cleanup = UnwindAction::Terminate(UnwindTerminateReason::Abi);
}
}

for id in calls_to_terminate {
let cleanup = body.basic_blocks_mut()[id].terminator_mut().unwind_mut().unwrap();
*cleanup = UnwindAction::Terminate(UnwindTerminateReason::Abi);
}

for id in cleanups_to_remove {
let cleanup = body.basic_blocks_mut()[id].terminator_mut().unwind_mut().unwrap();
*cleanup = UnwindAction::Unreachable;
}

// We may have invalidated some `cleanup` blocks so clean those up now.
super::simplify::remove_dead_blocks(body);
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/add_call_guards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ use rustc_middle::ty::TyCtxt;
use tracing::debug;

#[derive(PartialEq)]
pub enum AddCallGuards {
pub(super) enum AddCallGuards {
AllCallEdges,
CriticalCallEdges,
}
pub use self::AddCallGuards::*;
pub(super) use self::AddCallGuards::*;

/**
* Breaks outgoing critical edges for call terminators in the MIR.
Expand Down Expand Up @@ -37,7 +37,7 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards {
}

impl AddCallGuards {
pub fn add_call_guards(&self, body: &mut Body<'_>) {
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
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use crate::util;
///
/// The storage instructions are required to avoid stack space
/// blowup.
pub struct AddMovesForPackedDrops;
pub(super) struct AddMovesForPackedDrops;

impl<'tcx> crate::MirPass<'tcx> for AddMovesForPackedDrops {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Expand All @@ -44,7 +44,7 @@ impl<'tcx> crate::MirPass<'tcx> for AddMovesForPackedDrops {
}
}

pub fn add_moves_for_packed_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
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);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/add_retag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_hir::LangItem;
use rustc_middle::mir::*;
use rustc_middle::ty::{self, Ty, TyCtxt};

pub struct AddRetag;
pub(super) struct AddRetag;

/// Determine whether this type may contain a reference (or box), and thus needs retagging.
/// We will only recurse `depth` times into Tuples/ADTs to bound the cost of this.
Expand Down
9 changes: 4 additions & 5 deletions compiler/rustc_mir_transform/src/add_subtyping_projections.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
use rustc_index::IndexVec;
use rustc_middle::mir::patch::MirPatch;
use rustc_middle::mir::visit::MutVisitor;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;

pub struct Subtyper;
pub(super) struct Subtyper;

pub struct SubTypeChecker<'a, 'tcx> {
struct SubTypeChecker<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
patcher: MirPatch<'tcx>,
local_decls: &'a IndexVec<Local, LocalDecl<'tcx>>,
local_decls: &'a LocalDecls<'tcx>,
}

impl<'a, 'tcx> MutVisitor<'tcx> for SubTypeChecker<'a, 'tcx> {
Expand Down Expand Up @@ -52,7 +51,7 @@ 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;
pub fn subtype_finder<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
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 };

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/check_alignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt};
use rustc_session::Session;
use tracing::{debug, trace};

pub struct CheckAlignment;
pub(super) struct CheckAlignment;

impl<'tcx> crate::MirPass<'tcx> for CheckAlignment {
fn is_enabled(&self, sess: &Session) -> bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_span::Span;

use crate::errors;

pub struct CheckConstItemMutation;
pub(super) struct CheckConstItemMutation;

impl<'tcx> crate::MirLint<'tcx> for CheckConstItemMutation {
fn run_lint(&self, tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/check_packed_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_middle::ty::{self, TyCtxt};

use crate::{errors, util};

pub struct CheckPackedRef;
pub(super) struct CheckPackedRef;

impl<'tcx> crate::MirLint<'tcx> for CheckPackedRef {
fn run_lint(&self, tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc_middle::mir::{Body, BorrowKind, CastKind, Rvalue, StatementKind, Termi
use rustc_middle::ty::adjustment::PointerCoercion;
use rustc_middle::ty::TyCtxt;

pub struct CleanupPostBorrowck;
pub(super) struct CleanupPostBorrowck;

impl<'tcx> crate::MirPass<'tcx> for CleanupPostBorrowck {
fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/copy_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::ssa::SsaLocals;
/// where each of the locals is only assigned once.
///
/// We want to replace all those locals by `_a`, either copied or moved.
pub struct CopyProp;
pub(super) struct CopyProp;

impl<'tcx> crate::MirPass<'tcx> for CopyProp {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
Expand Down
15 changes: 4 additions & 11 deletions compiler/rustc_mir_transform/src/coroutine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
mod by_move_body;
use std::{iter, ops};

pub use by_move_body::coroutine_by_move_body_def_id;
pub(super) use by_move_body::coroutine_by_move_body_def_id;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::pluralize;
use rustc_hir as hir;
Expand Down Expand Up @@ -85,7 +85,7 @@ use tracing::{debug, instrument, trace};
use crate::deref_separator::deref_finder;
use crate::{abort_unwinding_calls, errors, pass_manager as pm, simplify};

pub struct StateTransform;
pub(super) struct StateTransform;

struct RenameLocalVisitor<'tcx> {
from: Local,
Expand Down Expand Up @@ -1199,7 +1199,7 @@ fn insert_panic_block<'tcx>(
message: AssertMessage<'tcx>,
) -> BasicBlock {
let assert_block = BasicBlock::new(body.basic_blocks.len());
let term = TerminatorKind::Assert {
let kind = TerminatorKind::Assert {
cond: Operand::Constant(Box::new(ConstOperand {
span: body.span,
user_ty: None,
Expand All @@ -1211,14 +1211,7 @@ fn insert_panic_block<'tcx>(
unwind: UnwindAction::Continue,
};

let source_info = SourceInfo::outermost(body.span);
body.basic_blocks_mut().push(BasicBlockData {
statements: Vec::new(),
terminator: Some(Terminator { source_info, kind: term }),
is_cleanup: false,
});

assert_block
insert_term_block(body, kind)
}

fn can_return<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, param_env: ty::ParamEnv<'tcx>) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/coroutine/by_move_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ use rustc_middle::ty::{self, InstanceKind, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::symbol::kw;
use rustc_target::abi::{FieldIdx, VariantIdx};

pub fn coroutine_by_move_body_def_id<'tcx>(
pub(crate) fn coroutine_by_move_body_def_id<'tcx>(
tcx: TyCtxt<'tcx>,
coroutine_def_id: LocalDefId,
) -> DefId {
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_mir_transform/src/cost_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const CONST_SWITCH_BONUS: usize = 10;

/// Verify that the callee body is compatible with the caller.
#[derive(Clone)]
pub(crate) struct CostChecker<'b, 'tcx> {
pub(super) struct CostChecker<'b, 'tcx> {
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
penalty: usize,
Expand All @@ -22,7 +22,7 @@ pub(crate) struct CostChecker<'b, 'tcx> {
}

impl<'b, 'tcx> CostChecker<'b, 'tcx> {
pub fn new(
pub(super) fn new(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
instance: Option<ty::Instance<'tcx>>,
Expand All @@ -36,7 +36,7 @@ impl<'b, 'tcx> CostChecker<'b, 'tcx> {
/// Needed because the `CostChecker` is used sometimes for just blocks,
/// and even the full `Inline` doesn't call `visit_body`, so there's nowhere
/// to put this logic in the visitor.
pub fn add_function_level_costs(&mut self) {
pub(super) fn add_function_level_costs(&mut self) {
fn is_call_like(bbd: &BasicBlockData<'_>) -> bool {
use TerminatorKind::*;
match bbd.terminator().kind {
Expand Down Expand Up @@ -64,7 +64,7 @@ impl<'b, 'tcx> CostChecker<'b, 'tcx> {
}
}

pub fn cost(&self) -> usize {
pub(super) fn cost(&self) -> usize {
usize::saturating_sub(self.penalty, self.bonus)
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pub mod query;
pub(super) mod query;

mod counters;
mod graph;
Expand Down Expand Up @@ -32,7 +32,7 @@ use crate::coverage::mappings::ExtractedMappings;
/// Inserts `StatementKind::Coverage` statements that either instrument the binary with injected
/// counters, via intrinsic `llvm.instrprof.increment`, and/or inject metadata used during codegen
/// to construct the coverage map.
pub struct InstrumentCoverage;
pub(super) struct InstrumentCoverage;

impl<'tcx> crate::MirPass<'tcx> for InstrumentCoverage {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/cross_crate_inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_span::sym;

use crate::{inline, pass_manager as pm};

pub fn provide(providers: &mut Providers) {
pub(super) fn provide(providers: &mut Providers) {
providers.cross_crate_inlinable = cross_crate_inlinable;
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/ctfe_limit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_middle::mir::{
use rustc_middle::ty::TyCtxt;
use tracing::instrument;

pub struct CtfeLimit;
pub(super) struct CtfeLimit;

impl<'tcx> crate::MirPass<'tcx> for CtfeLimit {
#[instrument(skip(self, _tcx, body))]
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use tracing::{debug, debug_span, instrument};
const BLOCK_LIMIT: usize = 100;
const PLACE_LIMIT: usize = 100;

pub struct DataflowConstProp;
pub(super) struct DataflowConstProp;

impl<'tcx> crate::MirPass<'tcx> for DataflowConstProp {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
Expand Down Expand Up @@ -332,7 +332,7 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> {
}

impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
pub fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, map: Map<'tcx>) -> Self {
fn new(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, map: Map<'tcx>) -> Self {
let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id());
Self {
map,
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/dead_store_elimination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::util::is_within_packed;
///
/// The `borrowed` set must be a `BitSet` of all the locals that are ever borrowed in this body. It
/// can be generated via the [`borrowed_locals`] function.
pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let borrowed_locals = borrowed_locals(body);

// If the user requests complete debuginfo, mark the locals that appear in it as live, so
Expand Down Expand Up @@ -127,7 +127,7 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
}
}

pub enum DeadStoreElimination {
pub(super) enum DeadStoreElimination {
Initial,
Final,
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/deduce_param_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ fn type_will_always_be_passed_directly(ty: Ty<'_>) -> bool {
/// body of the function instead of just the signature. These can be useful for optimization
/// purposes on a best-effort basis. We compute them here and store them into the crate metadata so
/// dependent crates can use them.
pub fn deduced_param_attrs<'tcx>(
pub(super) fn deduced_param_attrs<'tcx>(
tcx: TyCtxt<'tcx>,
def_id: LocalDefId,
) -> &'tcx [DeducedParamAttrs] {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/deduplicate_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use tracing::debug;

use super::simplify::simplify_cfg;

pub struct DeduplicateBlocks;
pub(super) struct DeduplicateBlocks;

impl<'tcx> crate::MirPass<'tcx> for DeduplicateBlocks {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
Expand Down
Loading
Loading