coverage: Store expression data in function coverage info

Even though expression details are now stored in the info structure, we still
need to inject `ExpressionUsed` statements into MIR, because if one is missing
during codegen then we know that it was optimized out and we can remap all of
its associated code regions to zero.

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

@@ -73,17 +73,6 @@ pub struct CounterExpression {
     pub rhs: Counter,
 }
 
-impl CounterExpression {
-    /// The dummy expression `(0 - 0)` has a representation of all zeroes,
-    /// making it marginally more efficient to initialize than `(0 + 0)`.
-    pub(crate) const DUMMY: Self =
-        Self { lhs: Counter::ZERO, kind: ExprKind::Subtract, rhs: Counter::ZERO };
-
-    pub fn new(lhs: Counter, kind: ExprKind, rhs: Counter) -> Self {
-        Self { kind, lhs, rhs }
-    }
-}
-
 /// Corresponds to enum `llvm::coverage::CounterMappingRegion::RegionKind`.
 ///
 /// Must match the layout of `LLVMRustCounterMappingRegionKind`.

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

@@ -2,19 +2,11 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind};
 
 use rustc_data_structures::fx::FxIndexSet;
 use rustc_index::bit_set::BitSet;
-use rustc_index::IndexVec;
 use rustc_middle::mir::coverage::{
-    CodeRegion, CounterId, CovTerm, ExpressionId, FunctionCoverageInfo, Mapping, Op,
+    CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, Op,
 };
 use rustc_middle::ty::Instance;
 
-#[derive(Clone, Debug, PartialEq)]
-pub struct Expression {
-    lhs: CovTerm,
-    op: Op,
-    rhs: CovTerm,
-}
-
 /// Holds all of the coverage mapping data associated with a function instance,
 /// collected during traversal of `Coverage` statements in the function's MIR.
 #[derive(Debug)]
@@ -26,7 +18,12 @@ pub struct FunctionCoverage<'tcx> {
     /// Tracks which counters have been seen, so that we can identify mappings
     /// to counters that were optimized out, and set them to zero.
     counters_seen: BitSet<CounterId>,
-    expressions: IndexVec<ExpressionId, Option<Expression>>,
+    /// Contains all expression IDs that have been seen in an `ExpressionUsed`
+    /// coverage statement, plus all expression IDs that aren't directly used
+    /// by any mappings (and therefore do not have expression-used statements).
+    /// After MIR traversal is finished, we can conclude that any IDs missing
+    /// from this set must have had their statements deleted by MIR opts.
+    expressions_seen: BitSet<ExpressionId>,
 }
 
 impl<'tcx> FunctionCoverage<'tcx> {
@@ -52,16 +49,30 @@ impl<'tcx> FunctionCoverage<'tcx> {
         is_used: bool,
     ) -> Self {
         let num_counters = function_coverage_info.num_counters;
-        let num_expressions = function_coverage_info.num_expressions;
+        let num_expressions = function_coverage_info.expressions.len();
         debug!(
             "FunctionCoverage::create(instance={instance:?}) has \
             num_counters={num_counters}, num_expressions={num_expressions}, is_used={is_used}"
         );
+
+        // Create a filled set of expression IDs, so that expressions not
+        // directly used by mappings will be treated as "seen".
+        // (If they end up being unused, LLVM will delete them for us.)
+        let mut expressions_seen = BitSet::new_filled(num_expressions);
+        // 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 `ExpressionUsed` statement during MIR traversal.
+        for Mapping { term, .. } in &function_coverage_info.mappings {
+            if let &CovTerm::Expression(id) = term {
+                expressions_seen.remove(id);
+            }
+        }
+
         Self {
             function_coverage_info,
             is_used,
             counters_seen: BitSet::new_empty(num_counters),
-            expressions: IndexVec::from_elem_n(None, num_expressions),
+            expressions_seen,
         }
     }
 
@@ -76,35 +87,10 @@ impl<'tcx> FunctionCoverage<'tcx> {
         self.counters_seen.insert(id);
     }
 
-    /// Adds information about a coverage expression.
+    /// Marks an expression ID as having been seen in an expression-used statement.
     #[instrument(level = "debug", skip(self))]
-    pub(crate) fn add_counter_expression(
+    pub(crate) fn mark_expression_id_seen(&mut self, id: ExpressionId) {
-        &mut self,
+        self.expressions_seen.insert(id);
-        expression_id: ExpressionId,
-        lhs: CovTerm,
-        op: Op,
-        rhs: CovTerm,
-    ) {
-        debug_assert!(
-            expression_id.as_usize() < self.expressions.len(),
-            "expression_id {} is out of range for expressions.len() = {}
-            for {:?}",
-            expression_id.as_usize(),
-            self.expressions.len(),
-            self,
-        );
-
-        let expression = Expression { lhs, op, rhs };
-        let slot = &mut self.expressions[expression_id];
-        match slot {
-            None => *slot = Some(expression),
-            // If this expression ID slot has already been filled, it should
-            // contain identical information.
-            Some(ref previous_expression) => assert_eq!(
-                previous_expression, &expression,
-                "add_counter_expression: expression for id changed"
-            ),
-        }
     }
 
     /// Identify expressions that will always have a value of zero, and note
@@ -125,13 +111,13 @@ impl<'tcx> FunctionCoverage<'tcx> {
         // 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, maybe_expression) in self.expressions.iter_enumerated() {
+        for (id, expression) in self.function_coverage_info.expressions.iter_enumerated() {
-            let Some(expression) = maybe_expression else {
+            if !self.expressions_seen.contains(id) {
-                // If an expression is missing, it must have been optimized away,
+                // 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
@@ -197,7 +183,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
         // Expression IDs are indices into `self.expressions`, and on the LLVM
         // side they will be treated as indices into `counter_expressions`, so
         // the two vectors should correspond 1:1.
-        assert_eq!(self.expressions.len(), counter_expressions.len());
+        assert_eq!(self.function_coverage_info.expressions.len(), counter_expressions.len());
 
         let counter_regions = self.counter_regions(zero_expressions);
 
@@ -217,27 +203,16 @@ impl<'tcx> FunctionCoverage<'tcx> {
             _ => Counter::from_term(operand),
         };
 
-        self.expressions
+        self.function_coverage_info
+            .expressions
             .iter()
-            .map(|expression| match expression {
+            .map(|&Expression { lhs, op, rhs }| CounterExpression {
-                None => {
+                lhs: counter_from_operand(lhs),
-                    // This expression ID was allocated, but we never saw the
+                kind: match op {
-                    // actual expression, so it must have been optimized out.
-                    // Replace it with a dummy expression, and let LLVM take
-                    // care of omitting it from the expression list.
-                    CounterExpression::DUMMY
-                }
-                &Some(Expression { lhs, op, rhs, .. }) => {
-                    // Convert the operands and operator as normal.
-                    CounterExpression::new(
-                        counter_from_operand(lhs),
-                        match op {
                     Op::Add => ExprKind::Add,
                     Op::Subtract => ExprKind::Subtract,
                 },
-                        counter_from_operand(rhs),
+                rhs: counter_from_operand(rhs),
-                    )
-                }
             })
             .collect::<Vec<_>>()
     }

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -145,8 +145,8 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
                 );
                 bx.instrprof_increment(fn_name, hash, num_counters, index);
             }
-            CoverageKind::Expression { id, lhs, op, rhs } => {
+            CoverageKind::ExpressionUsed { id } => {
-                func_coverage.add_counter_expression(id, lhs, op, rhs);
+                func_coverage.mark_expression_id_seen(id);
             }
         }
     }

compiler/rustc_middle/src/mir/coverage.rs

@@ -1,5 +1,6 @@
 //! Metadata from source code coverage analysis and instrumentation.
 
+use rustc_index::IndexVec;
 use rustc_macros::HashStable;
 use rustc_span::Symbol;
 
@@ -68,14 +69,16 @@ pub enum CoverageKind {
     /// If this statement does not survive MIR optimizations, any mappings that
     /// refer to this counter can have those references simplified to zero.
     CounterIncrement { id: CounterId },
-    Expression {
+
-        /// ID of this coverage-counter expression within its enclosing function.
+    /// Marks the point in MIR control-flow represented by a coverage expression.
-        /// Other expressions in the same function can refer to it as an operand.
+    ///
-        id: ExpressionId,
+    /// If this statement does not survive MIR optimizations, any mappings that
-        lhs: CovTerm,
+    /// refer to this expression can have those references simplified to zero.
-        op: Op,
+    ///
-        rhs: CovTerm,
+    /// (This is only inserted for expression IDs that are directly used by
-    },
+    /// mappings. Intermediate expressions with no direct mappings are
+    /// retained/zeroed based on whether they are transitively used.)
+    ExpressionUsed { id: ExpressionId },
 }
 
 impl Debug for CoverageKind {
@@ -83,17 +86,7 @@ impl Debug for CoverageKind {
         use CoverageKind::*;
         match self {
             CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()),
-            Expression { id, lhs, op, rhs } => write!(
+            ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()),
-                fmt,
-                "Expression({:?}) = {:?} {} {:?}",
-                id.index(),
-                lhs,
-                match op {
-                    Op::Add => "+",
-                    Op::Subtract => "-",
-                },
-                rhs,
-            ),
         }
     }
 }
@@ -135,6 +128,14 @@ impl Op {
     }
 }
 
+#[derive(Clone, Debug)]
+#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
+pub struct Expression {
+    pub lhs: CovTerm,
+    pub op: Op,
+    pub rhs: CovTerm,
+}
+
 #[derive(Clone, Debug)]
 #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
 pub struct Mapping {
@@ -157,7 +158,7 @@ pub struct Mapping {
 pub struct FunctionCoverageInfo {
     pub function_source_hash: u64,
     pub num_counters: usize,
-    pub num_expressions: usize,
 
+    pub expressions: IndexVec<ExpressionId, Expression>,
     pub mappings: Vec<Mapping>,
 }

compiler/rustc_middle/src/mir/pretty.rs

@@ -504,8 +504,11 @@ fn write_function_coverage_info(
     function_coverage_info: &coverage::FunctionCoverageInfo,
     w: &mut dyn io::Write,
 ) -> io::Result<()> {
-    let coverage::FunctionCoverageInfo { mappings, .. } = function_coverage_info;
+    let coverage::FunctionCoverageInfo { expressions, mappings, .. } = function_coverage_info;
 
+    for (id, expression) in expressions.iter_enumerated() {
+        writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?;
+    }
     for coverage::Mapping { term, code_region } in mappings {
         writeln!(w, "{INDENT}coverage {term:?} => {code_region:?};")?;
     }

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -19,7 +19,7 @@ const NESTED_INDENT: &str = "    ";
 #[derive(Clone)]
 pub(super) enum BcbCounter {
     Counter { id: CounterId },
-    Expression { id: ExpressionId, lhs: CovTerm, op: Op, rhs: CovTerm },
+    Expression { id: ExpressionId },
 }
 
 impl BcbCounter {
@@ -39,17 +39,7 @@ impl Debug for BcbCounter {
     fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             Self::Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()),
-            Self::Expression { id, lhs, op, rhs } => write!(
+            Self::Expression { id } => write!(fmt, "Expression({:?})", id.index()),
-                fmt,
-                "Expression({:?}) = {:?} {} {:?}",
-                id.index(),
-                lhs,
-                match op {
-                    Op::Add => "+",
-                    Op::Subtract => "-",
-                },
-                rhs,
-            ),
         }
     }
 }
@@ -58,7 +48,6 @@ impl Debug for BcbCounter {
 /// associated with nodes/edges in the BCB graph.
 pub(super) struct CoverageCounters {
     next_counter_id: CounterId,
-    next_expression_id: ExpressionId,
 
     /// Coverage counters/expressions that are associated with individual BCBs.
     bcb_counters: IndexVec<BasicCoverageBlock, Option<BcbCounter>>,
@@ -69,10 +58,9 @@ pub(super) struct CoverageCounters {
     /// Only used by debug assertions, to verify that BCBs with incoming edge
     /// counters do not have their own physical counters (expressions are allowed).
     bcb_has_incoming_edge_counters: BitSet<BasicCoverageBlock>,
-    /// Expression nodes that are not directly associated with any particular
+    /// Table of expression data, associating each expression ID with its
-    /// BCB/edge, but are needed as operands to more complex expressions.
+    /// corresponding operator (+ or -) and its LHS/RHS operands.
-    /// These are always [`BcbCounter::Expression`].
+    expressions: IndexVec<ExpressionId, Expression>,
-    pub(super) intermediate_expressions: Vec<BcbCounter>,
 }
 
 impl CoverageCounters {
@@ -81,12 +69,10 @@ impl CoverageCounters {
 
         Self {
             next_counter_id: CounterId::START,
-            next_expression_id: ExpressionId::START,
-
             bcb_counters: IndexVec::from_elem_n(None, num_bcbs),
             bcb_edge_counters: FxHashMap::default(),
             bcb_has_incoming_edge_counters: BitSet::new_empty(num_bcbs),
-            intermediate_expressions: Vec::new(),
+            expressions: IndexVec::new(),
         }
     }
 
@@ -107,8 +93,8 @@ impl CoverageCounters {
     }
 
     fn make_expression(&mut self, lhs: CovTerm, op: Op, rhs: CovTerm) -> BcbCounter {
-        let id = self.next_expression();
+        let id = self.expressions.push(Expression { lhs, op, rhs });
-        BcbCounter::Expression { id, lhs, op, rhs }
+        BcbCounter::Expression { id }
     }
 
     /// Counter IDs start from one and go up.
@@ -118,20 +104,13 @@ impl CoverageCounters {
         next
     }
 
-    /// Expression IDs start from 0 and go up.
-    /// (Counter IDs and Expression IDs are distinguished by the `Operand` enum.)
-    fn next_expression(&mut self) -> ExpressionId {
-        let next = self.next_expression_id;
-        self.next_expression_id = self.next_expression_id + 1;
-        next
-    }
-
     pub(super) fn num_counters(&self) -> usize {
         self.next_counter_id.as_usize()
     }
 
+    #[cfg(test)]
     pub(super) fn num_expressions(&self) -> usize {
-        self.next_expression_id.as_usize()
+        self.expressions.len()
     }
 
     fn set_bcb_counter(
@@ -207,6 +186,10 @@ impl CoverageCounters {
     ) -> impl Iterator<Item = ((BasicCoverageBlock, BasicCoverageBlock), BcbCounter)> + '_ {
         self.bcb_edge_counters.drain()
     }
+
+    pub(super) fn take_expressions(&mut self) -> IndexVec<ExpressionId, Expression> {
+        std::mem::take(&mut self.expressions)
+    }
 }
 
 /// Traverse the `CoverageGraph` and add either a `Counter` or `Expression` to every BCB, to be
@@ -333,7 +316,6 @@ impl<'a> MakeBcbCounters<'a> {
                     );
                     debug!("  [new intermediate expression: {:?}]", intermediate_expression);
                     let intermediate_expression_operand = intermediate_expression.as_term();
-                    self.coverage_counters.intermediate_expressions.push(intermediate_expression);
                     some_sumup_counter_operand.replace(intermediate_expression_operand);
                 }
             }
@@ -446,7 +428,6 @@ impl<'a> MakeBcbCounters<'a> {
                     intermediate_expression
                 );
                 let intermediate_expression_operand = intermediate_expression.as_term();
-                self.coverage_counters.intermediate_expressions.push(intermediate_expression);
                 some_sumup_edge_counter_operand.replace(intermediate_expression_operand);
             }
         }

compiler/rustc_mir_transform/src/coverage/mod.rs

@@ -167,9 +167,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
         // every coverage span has a `Counter` or `Expression` assigned to its `BasicCoverageBlock`
         // and all `Expression` dependencies (operands) are also generated, for any other
         // `BasicCoverageBlock`s not already associated with a coverage span.
-        //
-        // Intermediate expressions (used to compute other `Expression` values), which have no
-        // direct association with any `BasicCoverageBlock`, are accumulated inside `coverage_counters`.
         let bcb_has_coverage_spans = |bcb| coverage_spans.bcb_has_coverage_spans(bcb);
         let result = self
             .coverage_counters
@@ -195,29 +192,16 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
             // are in fact counted, even though they don't directly contribute to counting
             // their own independent code region's coverage.
             self.inject_indirect_counters();
-
-            // Intermediate expressions will be injected as the final step, after generating
-            // debug output, if any.
-            ////////////////////////////////////////////////////
         };
 
         if let Err(e) = result {
             bug!("Error processing: {:?}: {:?}", self.mir_body.source.def_id(), e.message)
         };
 
-        ////////////////////////////////////////////////////
-        // Finally, inject the intermediate expressions collected along the way.
-        for intermediate_expression in &self.coverage_counters.intermediate_expressions {
-            inject_intermediate_expression(
-                self.mir_body,
-                self.make_mir_coverage_kind(intermediate_expression),
-            );
-        }
-
         self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo {
             function_source_hash: self.function_source_hash,
             num_counters: self.coverage_counters.num_counters(),
-            num_expressions: self.coverage_counters.num_expressions(),
+            expressions: self.coverage_counters.take_expressions(),
             mappings: std::mem::take(&mut self.mappings),
         }));
     }
@@ -304,10 +288,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                         inject_to_bb,
                     );
                 }
-                BcbCounter::Expression { .. } => inject_intermediate_expression(
+                // Experessions with no associated spans don't need to inject a statement.
-                    self.mir_body,
+                BcbCounter::Expression { .. } => {}
-                    self.make_mir_coverage_kind(&counter_kind),
-                ),
             }
         }
     }
@@ -330,9 +312,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
     fn make_mir_coverage_kind(&self, counter_kind: &BcbCounter) -> CoverageKind {
         match *counter_kind {
             BcbCounter::Counter { id } => CoverageKind::CounterIncrement { id },
-            BcbCounter::Expression { id, lhs, op, rhs } => {
+            BcbCounter::Expression { id } => CoverageKind::ExpressionUsed { id },
-                CoverageKind::Expression { id, lhs, op, rhs }
-            }
         }
     }
 }
@@ -371,20 +351,6 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb
     data.statements.insert(0, statement);
 }
 
-// Non-code expressions are injected into the coverage map, without generating executable code.
-fn inject_intermediate_expression(mir_body: &mut mir::Body<'_>, expression: CoverageKind) {
-    debug_assert!(matches!(expression, CoverageKind::Expression { .. }));
-    debug!("  injecting non-code expression {:?}", expression);
-    let inject_in_bb = mir::START_BLOCK;
-    let data = &mut mir_body[inject_in_bb];
-    let source_info = data.terminator().source_info;
-    let statement = Statement {
-        source_info,
-        kind: StatementKind::Coverage(Box::new(Coverage { kind: expression })),
-    };
-    data.statements.push(statement);
-}
-
 /// Convert the Span into its file name, start line and column, and end line and column
 fn make_code_region(
     source_map: &SourceMap,

compiler/rustc_mir_transform/src/coverage/tests.rs

@@ -656,7 +656,7 @@ fn test_make_bcb_counters() {
         coverage_counters
             .make_bcb_counters(&mut basic_coverage_blocks, bcb_has_coverage_spans)
             .expect("should be Ok");
-        assert_eq!(coverage_counters.intermediate_expressions.len(), 0);
+        assert_eq!(coverage_counters.num_expressions(), 0);
 
         let_bcb!(1);
         assert_eq!(

tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff

@@ -7,6 +7,8 @@
       let mut _2: bool;
       let mut _3: !;
   
++     coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Add, rhs: Counter(1) };
++     coverage ExpressionId(1) => Expression { lhs: Expression(0), op: Subtract, rhs: Counter(1) };
 +     coverage Counter(0) => /the/src/instrument_coverage.rs:11:1 - 11:11;
 +     coverage Expression(0) => /the/src/instrument_coverage.rs:12:5 - 13:17;
 +     coverage Expression(1) => /the/src/instrument_coverage.rs:14:13 - 14:18;
@@ -19,7 +21,7 @@
       }
   
       bb1: {
-+         Coverage::Expression(0) = Counter(0) + Counter(1);
++         Coverage::ExpressionUsed(0);
           falseUnwind -> [real: bb2, unwind: bb6];
       }
   
@@ -33,7 +35,7 @@
       }
   
       bb4: {
-+         Coverage::Expression(1) = Expression(0) - Counter(1);
++         Coverage::ExpressionUsed(1);
           _0 = const ();
           StorageDead(_2);
           return;