Skip to content

Commit

Permalink
coverage: Remove the old code for simplifying counters after MIR opts
Browse files Browse the repository at this point in the history
  • Loading branch information
@Zalathar
Zalathar committed Jan 25, 2025
1 parent 6beabf0 commit 4cb2f28
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 183 deletions.
25 changes: 9 additions & 16 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub(crate) fn prepare_covfun_record<'tcx>(
let fn_cov_info = tcx.instance_mir(instance.def).function_coverage_info.as_deref()?;
let ids_info = tcx.coverage_ids_info(instance.def)?;

let expressions = prepare_expressions(ids_info, is_used);
let expressions = prepare_expressions(ids_info);

let mut covfun = CovfunRecord {
mangled_function_name: tcx.symbol_name(instance).name,
Expand All @@ -76,16 +76,8 @@ pub(crate) fn prepare_covfun_record<'tcx>(
}

/// Convert the function's coverage-counter expressions into a form suitable for FFI.
fn prepare_expressions(ids_info: &CoverageIdsInfo, is_used: bool) -> Vec<ffi::CounterExpression> {
// If any counters or expressions were removed by MIR opts, replace their
// terms with zero.
let counter_for_term = |term| {
if !is_used || ids_info.is_zero_term(term) {
ffi::Counter::ZERO
} else {
ffi::Counter::from_term(term)
}
};
fn prepare_expressions(ids_info: &CoverageIdsInfo) -> Vec<ffi::CounterExpression> {
let counter_for_term = ffi::Counter::from_term;

// We know that LLVM will optimize out any unused expressions before
// producing the final coverage map, so there's no need to do the same
Expand Down Expand Up @@ -133,13 +125,14 @@ fn fill_region_tables<'tcx>(

// For each counter/region pair in this function+file, convert it to a
// form suitable for FFI.
let is_zero_term = |term| !covfun.is_used || ids_info.is_zero_term(term);
for &Mapping { ref kind, span } in &fn_cov_info.mappings {
// If the mapping refers to counters/expressions that were removed by
// MIR opts, replace those occurrences with zero.
// If this function is unused, replace all counters with zero.
let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter {
let term = ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term");
let term = if is_zero_term(term) { CovTerm::Zero } else { term };
let term = if covfun.is_used {
ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term")
} else {
CovTerm::Zero
};
ffi::Counter::from_term(term)
};

Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,9 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
CoverageKind::VirtualCounter { bcb }
if let Some(&id) = ids_info.phys_counter_for_node.get(&bcb) =>
{
let num_counters = ids_info.num_counters_after_mir_opts();

let fn_name = bx.get_pgo_func_name_var(instance);
let hash = bx.const_u64(function_coverage_info.function_source_hash);
let num_counters = bx.const_u32(num_counters);
let num_counters = bx.const_u32(ids_info.num_counters);
let index = bx.const_u32(id.as_u32());
debug!(
"codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})",
Expand Down
32 changes: 1 addition & 31 deletions compiler/rustc_middle/src/mir/coverage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use std::fmt::{self, Debug, Formatter};

use rustc_data_structures::fx::FxIndexMap;
use rustc_index::bit_set::DenseBitSet;
use rustc_index::{Idx, IndexVec};
use rustc_macros::{HashStable, TyDecodable, TyEncodable};
use rustc_span::Span;
Expand Down Expand Up @@ -278,41 +277,12 @@ pub struct MCDCDecisionSpan {
/// Used by the `coverage_ids_info` query.
#[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable)]
pub struct CoverageIdsInfo {
pub counters_seen: DenseBitSet<CounterId>,
pub zero_expressions: DenseBitSet<ExpressionId>,

pub num_counters: u32,
pub phys_counter_for_node: FxIndexMap<BasicCoverageBlock, CounterId>,
pub term_for_bcb: IndexVec<BasicCoverageBlock, Option<CovTerm>>,
pub expressions: IndexVec<ExpressionId, Expression>,
}

impl CoverageIdsInfo {
/// Coverage codegen needs to know how many coverage counters are ever
/// incremented within a function, so that it can set the `num-counters`
/// argument of the `llvm.instrprof.increment` intrinsic.
///
/// This may be less than the highest counter ID emitted by the
/// InstrumentCoverage MIR pass, if the highest-numbered counter increments
/// were removed by MIR optimizations.
pub fn num_counters_after_mir_opts(&self) -> u32 {
// FIXME(Zalathar): Currently this treats an unused counter as "used"
// if its ID is less than that of the highest counter that really is
// used. Fixing this would require adding a renumbering step somewhere.
self.counters_seen.last_set_in(..).map_or(0, |max| max.as_u32() + 1)
}

/// Returns `true` if the given term is known to have a value of zero, taking
/// into account knowledge of which counters are unused and which expressions
/// are always zero.
pub fn is_zero_term(&self, term: CovTerm) -> bool {
match term {
CovTerm::Zero => true,
CovTerm::Counter(id) => !self.counters_seen.contains(id),
CovTerm::Expression(id) => self.zero_expressions.contains(id),
}
}
}

rustc_index::newtype_index! {
/// During the `InstrumentCoverage` MIR pass, a BCB is a node in the
/// "coverage graph", which is a refinement of the MIR control-flow graph
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/coverage/counters.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub(super) struct CoverageCounters {
/// List of places where a counter-increment statement should be injected
/// into MIR, each with its corresponding counter ID.
pub(crate) phys_counter_for_node: FxIndexMap<BasicCoverageBlock, CounterId>,
next_counter_id: CounterId,
pub(crate) next_counter_id: CounterId,

/// Coverage counters/expressions that are associated with individual BCBs.
pub(crate) node_counters: IndexVec<BasicCoverageBlock, Option<CovTerm>>,
Expand Down
137 changes: 5 additions & 132 deletions compiler/rustc_mir_transform/src/coverage/query.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
use rustc_data_structures::captures::Captures;
use rustc_index::IndexSlice;
use rustc_index::bit_set::DenseBitSet;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::coverage::{
BasicCoverageBlock, CounterId, CovTerm, CoverageIdsInfo, CoverageKind, Expression,
ExpressionId, MappingKind, Op,
};
use rustc_middle::mir::coverage::{BasicCoverageBlock, CoverageIdsInfo, CoverageKind, MappingKind};
use rustc_middle::mir::{Body, Statement, StatementKind};
use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::{self, TyCtxt};
Expand Down Expand Up @@ -136,44 +132,12 @@ fn coverage_ids_info<'tcx>(
let node_counters = make_node_counters(&fn_cov_info.node_flow_data, &fn_cov_info.priority_list);
let coverage_counters = transcribe_counters(&node_counters, &bcb_needs_counter, &bcbs_seen);

let mut counters_seen = DenseBitSet::new_empty(coverage_counters.node_counters.len());
let mut expressions_seen = DenseBitSet::new_filled(coverage_counters.expressions.len());

// For each expression ID that is directly used by one or more mappings,
// mark it as not-yet-seen. This indicates that we expect to see a
// corresponding `VirtualCounter` statement during MIR traversal.
for mapping in fn_cov_info.mappings.iter() {
// Currently we only worry about ordinary code mappings.
// For branch and MC/DC mappings, expressions might not correspond
// to any particular point in the control-flow graph.
if let MappingKind::Code { bcb } = mapping.kind
&& let Some(CovTerm::Expression(id)) = coverage_counters.node_counters[bcb]
{
expressions_seen.remove(id);
}
}

for bcb in bcbs_seen.iter() {
if let Some(&id) = coverage_counters.phys_counter_for_node.get(&bcb) {
counters_seen.insert(id);
}
if let Some(CovTerm::Expression(id)) = coverage_counters.node_counters[bcb] {
expressions_seen.insert(id);
}
}

let zero_expressions = identify_zero_expressions(
&coverage_counters.expressions,
&counters_seen,
&expressions_seen,
);

let CoverageCounters { phys_counter_for_node, node_counters, expressions, .. } =
coverage_counters;
let CoverageCounters {
phys_counter_for_node, next_counter_id, node_counters, expressions, ..
} = coverage_counters;

Some(CoverageIdsInfo {
counters_seen,
zero_expressions,
num_counters: next_counter_id.as_u32(),
phys_counter_for_node,
term_for_bcb: node_counters,
expressions,
Expand All @@ -195,94 +159,3 @@ fn is_inlined(body: &Body<'_>, statement: &Statement<'_>) -> bool {
let scope_data = &body.source_scopes[statement.source_info.scope];
scope_data.inlined.is_some() || scope_data.inlined_parent_scope.is_some()
}

/// Identify expressions that will always have a value of zero, and note their
/// IDs in a `DenseBitSet`. Mappings that refer to a zero expression can instead
/// become mappings to a constant zero value.
///
/// This function mainly exists to preserve the simplifications that were
/// already being performed by the Rust-side expression renumbering, so that
/// the resulting coverage mappings don't get worse.
fn identify_zero_expressions(
expressions: &IndexSlice<ExpressionId, Expression>,
counters_seen: &DenseBitSet<CounterId>,
expressions_seen: &DenseBitSet<ExpressionId>,
) -> DenseBitSet<ExpressionId> {
// The set of expressions that either were optimized out entirely, or
// have zero as both of their operands, and will therefore always have
// a value of zero. Other expressions that refer to these as operands
// can have those operands replaced with `CovTerm::Zero`.
let mut zero_expressions = DenseBitSet::new_empty(expressions.len());

// Simplify a copy of each expression based on lower-numbered expressions,
// and then update the set of always-zero expressions if necessary.
// (By construction, expressions can only refer to other expressions
// that have lower IDs, so one pass is sufficient.)
for (id, expression) in expressions.iter_enumerated() {
if !expressions_seen.contains(id) {
// If an expression was not seen, it must have been optimized away,
// so any operand that refers to it can be replaced with zero.
zero_expressions.insert(id);
continue;
}

// We don't need to simplify the actual expression data in the
// expressions list; we can just simplify a temporary copy and then
// use that to update the set of always-zero expressions.
let Expression { mut lhs, op, mut rhs } = *expression;

// If an expression has an operand that is also an expression, the
// operand's ID must be strictly lower. This is what lets us find
// all zero expressions in one pass.
let assert_operand_expression_is_lower = |operand_id: ExpressionId| {
assert!(
operand_id < id,
"Operand {operand_id:?} should be less than {id:?} in {expression:?}",
)
};

// If an operand refers to a counter or expression that is always
// zero, then that operand can be replaced with `CovTerm::Zero`.
let maybe_set_operand_to_zero = |operand: &mut CovTerm| {
if let CovTerm::Expression(id) = *operand {
assert_operand_expression_is_lower(id);
}

if is_zero_term(&counters_seen, &zero_expressions, *operand) {
*operand = CovTerm::Zero;
}
};
maybe_set_operand_to_zero(&mut lhs);
maybe_set_operand_to_zero(&mut rhs);

// Coverage counter values cannot be negative, so if an expression
// involves subtraction from zero, assume that its RHS must also be zero.
// (Do this after simplifications that could set the LHS to zero.)
if lhs == CovTerm::Zero && op == Op::Subtract {
rhs = CovTerm::Zero;
}

// After the above simplifications, if both operands are zero, then
// we know that this expression is always zero too.
if lhs == CovTerm::Zero && rhs == CovTerm::Zero {
zero_expressions.insert(id);
}
}

zero_expressions
}

/// Returns `true` if the given term is known to have a value of zero, taking
/// into account knowledge of which counters are unused and which expressions
/// are always zero.
fn is_zero_term(
counters_seen: &DenseBitSet<CounterId>,
zero_expressions: &DenseBitSet<ExpressionId>,
term: CovTerm,
) -> bool {
match term {
CovTerm::Zero => true,
CovTerm::Counter(id) => !counters_seen.contains(id),
CovTerm::Expression(id) => zero_expressions.contains(id),
}
}

0 comments on commit 4cb2f28

Please sign in to comment.