coverage: Remove the old code for simplifying counters after MIR opts

Author: Zalathar

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen/covfun.rs

@@ -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,
@@ -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
@@ -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)
         };
 

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -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={:?})",

compiler/rustc_middle/src/mir/coverage.rs

@@ -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;
@@ -277,41 +276,12 @@ pub struct MCDCDecisionSpan {
 /// Returned 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

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -135,7 +135,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>>,

compiler/rustc_mir_transform/src/coverage/query.rs

@@ -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::ty::{self, TyCtxt};
 use rustc_middle::util::Providers;
@@ -134,44 +130,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,
@@ -193,94 +157,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),
-    }
-}