Skip to content

Cheaper dump_mir #105083

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

Closed
wants to merge 2 commits into from
Closed
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
17 changes: 12 additions & 5 deletions compiler/rustc_borrowck/src/nll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use rustc_data_structures::vec_map::VecMap;
use rustc_hir::def_id::LocalDefId;
use rustc_index::vec::IndexVec;
use rustc_infer::infer::InferCtxt;
use rustc_middle::mir::{create_dump_file, dump_enabled, dump_mir, PassWhere};
use rustc_middle::mir::{
create_dump_file, dump_mir, maybe_dump_mir, pass_name_matches_dump_filters, PassWhere,
};
use rustc_middle::mir::{
BasicBlock, Body, ClosureOutlivesSubject, ClosureRegionRequirements, LocalKind, Location,
Promoted,
Expand Down Expand Up @@ -73,7 +75,7 @@ pub(crate) fn replace_regions_in_mir<'tcx>(
// Replace all remaining regions with fresh inference variables.
renumber::renumber_mir(infcx, body, promoted);

dump_mir(infcx.tcx, None, "renumber", &0, body, |_, _| Ok(()));
maybe_dump_mir(infcx.tcx, None, "renumber", &0, body, |_, _| Ok(()));

universal_regions
}
Expand Down Expand Up @@ -327,11 +329,16 @@ pub(super) fn dump_mir_results<'tcx>(
regioncx: &RegionInferenceContext<'tcx>,
closure_region_requirements: &Option<ClosureRegionRequirements<'_>>,
) {
if !dump_enabled(infcx.tcx, "nll", body.source.def_id()) {
let pass_name = "nll";
if let Some(filters) = &infcx.tcx.sess.opts.unstable_opts.dump_mir
&& pass_name_matches_dump_filters(infcx.tcx, filters, pass_name, body.source.def_id())
{
// continue
} else {
return;
}
};

dump_mir(infcx.tcx, None, "nll", &0, body, |pass_where, out| {
dump_mir(infcx.tcx, None, pass_name, &0, body, |pass_where, out| {
match pass_where {
// Before the CFG, dump out the values for each region variable.
PassWhere::BeforeCFG => {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ pub mod visit;
pub use self::generic_graph::graphviz_safe_def_name;
pub use self::graphviz::write_mir_graphviz;
pub use self::pretty::{
create_dump_file, display_allocation, dump_enabled, dump_mir, write_mir_pretty, PassWhere,
create_dump_file, display_allocation, dump_mir, maybe_dump_mir, pass_name_matches_dump_filters,
write_mir_pretty, PassWhere,
};

/// Types for locals
Expand Down
54 changes: 28 additions & 26 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub enum PassWhere {
/// - `foo & nll | bar & typeck` == match if `foo` and `nll` both appear in the name
/// or `typeck` and `bar` both appear in the name.
#[inline]
pub fn dump_mir<'tcx, F>(
pub fn maybe_dump_mir<'tcx, F>(
tcx: TyCtxt<'tcx>,
pass_num: Option<&dyn Display>,
pass_name: &str,
Expand All @@ -82,34 +82,14 @@ pub fn dump_mir<'tcx, F>(
) where
F: FnMut(PassWhere, &mut dyn Write) -> io::Result<()>,
{
if !dump_enabled(tcx, pass_name, body.source.def_id()) {
return;
if let Some(filters) = &tcx.sess.opts.unstable_opts.dump_mir {
if pass_name_matches_dump_filters(tcx, filters, pass_name, body.source.def_id()) {
dump_mir(tcx, pass_num, pass_name, disambiguator, body, extra_data);
}
}

dump_matched_mir_node(tcx, pass_num, pass_name, disambiguator, body, extra_data);
}

pub fn dump_enabled<'tcx>(tcx: TyCtxt<'tcx>, pass_name: &str, def_id: DefId) -> bool {
let Some(ref filters) = tcx.sess.opts.unstable_opts.dump_mir else {
return false;
};
Comment on lines -93 to -95
Copy link
Contributor

Choose a reason for hiding this comment

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

is it actually perf relevant to do this at every call site instead of inside the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, as the perf CI results show.

There is a two-step check when deciding whether to dump MIR.

  • Is -Zdump-mir specified?
  • If so, does the MIR pass name match the -Zdump-mir value?

If both those conditions pass, then the MIR is dumped, which involves printing pass_name and pass_num.

For most of the dump_mir callsites pass_name and pass_num are simple scalars, which poses no problem.
But at at two of the dump_mir call sites (in dump_mir_for_phase_change and dump_mir_for_pass) pass_name and pass_num are constructed in a way that is moderately expensive, and this happens before the two-step check.

The PR changes things so that, at two of these call sites, pass_name is only constructed if the first part of the two-step check succeeds, and pass_num is only constructed if the second part of the two-step check succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried building an alternative design (#105121) that afaict should end up with similar perf improvements.

// see notes on #41697 below
let node_path = ty::print::with_forced_impl_filename_line!(tcx.def_path_str(def_id));
filters.split('|').any(|or_filter| {
or_filter.split('&').all(|and_filter| {
let and_filter_trimmed = and_filter.trim();
and_filter_trimmed == "all"
|| pass_name.contains(and_filter_trimmed)
|| node_path.contains(and_filter_trimmed)
})
})
}

// #41697 -- we use `with_forced_impl_filename_line()` because
// `def_path_str()` would otherwise trigger `type_of`, and this can
// run while we are already attempting to evaluate `type_of`.

fn dump_matched_mir_node<'tcx, F>(
pub fn dump_mir<'tcx, F>(
tcx: TyCtxt<'tcx>,
pass_num: Option<&dyn Display>,
pass_name: &str,
Expand All @@ -119,6 +99,10 @@ fn dump_matched_mir_node<'tcx, F>(
) where
F: FnMut(PassWhere, &mut dyn Write) -> io::Result<()>,
{
// #41697 -- we use `with_forced_impl_filename_line()` because
// `def_path_str()` would otherwise trigger `type_of`, and this can
// run while we are already attempting to evaluate `type_of`.

let _: io::Result<()> = try {
let mut file =
create_dump_file(tcx, "mir", pass_num, pass_name, disambiguator, body.source)?;
Expand Down Expand Up @@ -161,6 +145,24 @@ fn dump_matched_mir_node<'tcx, F>(
}
}

pub fn pass_name_matches_dump_filters<'tcx>(
tcx: TyCtxt<'tcx>,
filters: &str,
pass_name: &str,
def_id: DefId,
) -> bool {
// see notes on #41697 below
let node_path = ty::print::with_forced_impl_filename_line!(tcx.def_path_str(def_id));
filters.split('|').any(|or_filter| {
or_filter.split('&').all(|and_filter| {
let and_filter_trimmed = and_filter.trim();
and_filter_trimmed == "all"
|| pass_name.contains(and_filter_trimmed)
|| node_path.contains(and_filter_trimmed)
})
})
}

/// Returns the file basename portion (without extension) of a filename path
/// where we should dump a MIR representation output files.
fn dump_file_basename<'tcx>(
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_mir_dataflow/src/framework/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_hir::def_id::DefId;
use rustc_index::bit_set::BitSet;
use rustc_index::vec::{Idx, IndexVec};
use rustc_middle::mir::{self, traversal, BasicBlock};
use rustc_middle::mir::{create_dump_file, dump_enabled};
use rustc_middle::mir::{create_dump_file, pass_name_matches_dump_filters};
use rustc_middle::ty::TyCtxt;
use rustc_span::symbol::{sym, Symbol};

Expand Down Expand Up @@ -292,7 +292,8 @@ where
}

None if tcx.sess.opts.unstable_opts.dump_mir_dataflow
&& dump_enabled(tcx, A::NAME, def_id) =>
&& let Some(filters) = &tcx.sess.opts.unstable_opts.dump_mir
&& pass_name_matches_dump_filters(tcx, filters, A::NAME, def_id) =>
{
create_dump_file(
tcx,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_mir_dataflow/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#![feature(associated_type_defaults)]
#![feature(box_patterns)]
#![feature(exact_size_is_empty)]
#![feature(if_let_guard)]
#![feature(let_chains)]
#![feature(min_specialization)]
#![feature(once_cell)]
#![feature(stmt_expr_attributes)]
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_mir_transform/src/coverage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_index::vec::IndexVec;
use rustc_middle::hir;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::coverage::*;
use rustc_middle::mir::dump_enabled;
use rustc_middle::mir::pass_name_matches_dump_filters;
use rustc_middle::mir::{
self, BasicBlock, BasicBlockData, Coverage, SourceInfo, Statement, StatementKind, Terminator,
TerminatorKind,
Expand Down Expand Up @@ -159,7 +159,13 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
let mut graphviz_data = debug::GraphvizData::new();
let mut debug_used_expressions = debug::UsedExpressions::new();

let dump_mir = dump_enabled(tcx, self.pass_name, def_id);
let dump_mir = if let Some(filters) = &tcx.sess.opts.unstable_opts.dump_mir
&& pass_name_matches_dump_filters(tcx, filters, self.pass_name, def_id)
{
true
} else {
false
};
let dump_graphviz = dump_mir && tcx.sess.opts.unstable_opts.dump_mir_graphviz;
let dump_spanview = dump_mir && tcx.sess.opts.unstable_opts.dump_mir_spanview.is_some();

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ use std::collections::hash_map::{Entry, OccupiedEntry};
use crate::MirPass;
use rustc_data_structures::fx::FxHashMap;
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::{dump_mir, PassWhere};
use rustc_middle::mir::{maybe_dump_mir, PassWhere};
use rustc_middle::mir::{
traversal, BasicBlock, Body, InlineAsmOperand, Local, LocalKind, Location, Operand, Place,
Rvalue, Statement, StatementKind, TerminatorKind,
Expand Down Expand Up @@ -787,7 +787,7 @@ fn dest_prop_mir_dump<'body, 'tcx>(
round: usize,
) {
let mut reachable = None;
dump_mir(tcx, None, "DestinationPropagation-dataflow", &round, body, |pass_where, w| {
maybe_dump_mir(tcx, None, "DestinationPropagation-dataflow", &round, body, |pass_where, w| {
let reachable = reachable.get_or_insert_with(|| traversal::reachable_as_bitset(body));

match pass_where {
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_mir_transform/src/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ use rustc_hir::lang_items::LangItem;
use rustc_hir::GeneratorKind;
use rustc_index::bit_set::{BitMatrix, BitSet, GrowableBitSet};
use rustc_index::vec::{Idx, IndexVec};
use rustc_middle::mir::dump_mir;
use rustc_middle::mir::maybe_dump_mir;
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt};
Expand Down Expand Up @@ -1000,7 +1000,7 @@ fn create_generator_drop_shim<'tcx>(
// unrelated code from the resume part of the function
simplify::remove_dead_blocks(tcx, &mut body);

dump_mir(tcx, None, "generator_drop", &0, &body, |_, _| Ok(()));
maybe_dump_mir(tcx, None, "generator_drop", &0, &body, |_, _| Ok(()));

body
}
Expand Down Expand Up @@ -1171,7 +1171,7 @@ fn create_generator_resume_function<'tcx>(
// unrelated code from the drop part of the function
simplify::remove_dead_blocks(tcx, body);

dump_mir(tcx, None, "generator_resume", &0, body, |_, _| Ok(()));
maybe_dump_mir(tcx, None, "generator_resume", &0, body, |_, _| Ok(()));
}

fn insert_clean_drop(body: &mut Body<'_>) -> BasicBlock {
Expand Down Expand Up @@ -1394,14 +1394,14 @@ impl<'tcx> MirPass<'tcx> for StateTransform {
// This is expanded to a drop ladder in `elaborate_generator_drops`.
let drop_clean = insert_clean_drop(body);

dump_mir(tcx, None, "generator_pre-elab", &0, body, |_, _| Ok(()));
maybe_dump_mir(tcx, None, "generator_pre-elab", &0, body, |_, _| Ok(()));

// Expand `drop(generator_struct)` to a drop ladder which destroys upvars.
// If any upvars are moved out of, drop elaboration will handle upvar destruction.
// However we need to also elaborate the code generated by `insert_clean_drop`.
elaborate_generator_drops(tcx, body);

dump_mir(tcx, None, "generator_post-transform", &0, body, |_, _| Ok(()));
maybe_dump_mir(tcx, None, "generator_post-transform", &0, body, |_, _| Ok(()));

// Create a copy of our MIR and use it to create the drop shim for the generator
let drop_shim = create_generator_drop_shim(tcx, &transform, gen_ty, body, drop_clean);
Expand Down
47 changes: 26 additions & 21 deletions compiler/rustc_mir_transform/src/pass_manager.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::borrow::Cow;

use rustc_middle::mir::{self, Body, MirPhase, RuntimePhase};
use rustc_middle::mir::{self, pass_name_matches_dump_filters, Body, MirPhase, RuntimePhase};
use rustc_middle::ty::TyCtxt;
use rustc_session::Session;

Expand Down Expand Up @@ -166,27 +166,32 @@ pub fn dump_mir_for_pass<'tcx>(
pass_name: &str,
is_after: bool,
) {
let phase_index = body.phase.phase_index();

mir::dump_mir(
tcx,
Some(&format_args!("{:03}-{:03}", phase_index, body.pass_count)),
pass_name,
if is_after { &"after" } else { &"before" },
body,
|_, _| Ok(()),
);
if let Some(filters) = &tcx.sess.opts.unstable_opts.dump_mir {
if pass_name_matches_dump_filters(tcx, filters, pass_name, body.source.def_id()) {
mir::dump_mir(
tcx,
Some(&format_args!("{:03}-{:03}", body.phase.phase_index(), body.pass_count)),
pass_name,
if is_after { &"after" } else { &"before" },
body,
|_, _| Ok(()),
);
}
}
}

pub fn dump_mir_for_phase_change<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
let phase_index = body.phase.phase_index();

mir::dump_mir(
tcx,
Some(&format_args!("{:03}-000", phase_index)),
&format!("{}", body.phase),
&"after",
body,
|_, _| Ok(()),
)
if let Some(filters) = &tcx.sess.opts.unstable_opts.dump_mir {
let pass_name = &format!("{}", body.phase);
if pass_name_matches_dump_filters(tcx, filters, pass_name, body.source.def_id()) {
mir::dump_mir(
tcx,
Some(&format_args!("{:03}-000", body.phase.phase_index())),
pass_name,
&"after",
body,
|_, _| Ok(()),
)
}
}
}