diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index fe334b292657d..763186a58bf9f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -1,4 +1,4 @@ -use rustc_middle::mir::coverage::{CounterId, MappedExpressionIndex}; +use rustc_middle::mir::coverage::{CounterId, ExpressionId, Operand}; /// Must match the layout of `LLVMRustCounterKind`. #[derive(Copy, Clone, Debug)] @@ -39,20 +39,16 @@ impl Counter { } /// Constructs a new `Counter` of kind `Expression`. - pub fn expression(mapped_expression_index: MappedExpressionIndex) -> Self { - Self { kind: CounterKind::Expression, id: mapped_expression_index.into() } + pub(crate) fn expression(expression_id: ExpressionId) -> Self { + Self { kind: CounterKind::Expression, id: expression_id.as_u32() } } - /// Returns true if the `Counter` kind is `Zero`. - pub fn is_zero(&self) -> bool { - matches!(self.kind, CounterKind::Zero) - } - - /// An explicitly-named function to get the ID value, making it more obvious - /// that the stored value is now 0-based. - pub fn zero_based_id(&self) -> u32 { - debug_assert!(!self.is_zero(), "`id` is undefined for CounterKind::Zero"); - self.id + pub(crate) fn from_operand(operand: Operand) -> Self { + match operand { + Operand::Zero => Self::ZERO, + Operand::Counter(id) => Self::counter_value_reference(id), + Operand::Expression(id) => Self::expression(id), + } } } @@ -78,6 +74,11 @@ pub struct CounterExpression { } 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 } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 31fcbbad0e607..e83110dcad460 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,11 +1,8 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; use rustc_data_structures::fx::FxIndexSet; -use rustc_index::{IndexSlice, IndexVec}; -use rustc_middle::bug; -use rustc_middle::mir::coverage::{ - CodeRegion, CounterId, ExpressionId, MappedExpressionIndex, Op, Operand, -}; +use rustc_index::IndexVec; +use rustc_middle::mir::coverage::{CodeRegion, CounterId, ExpressionId, Op, Operand}; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -199,8 +196,14 @@ impl<'tcx> FunctionCoverage<'tcx> { self.instance ); + let counter_expressions = self.counter_expressions(); + // 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()); + let counter_regions = self.counter_regions(); - let (counter_expressions, expression_regions) = self.expressions_with_regions(); + let expression_regions = self.expression_regions(); let unreachable_regions = self.unreachable_regions(); let counter_regions = @@ -216,146 +219,50 @@ impl<'tcx> FunctionCoverage<'tcx> { }) } - fn expressions_with_regions( - &self, - ) -> (Vec, impl Iterator) { - let mut counter_expressions = Vec::with_capacity(self.expressions.len()); - let mut expression_regions = Vec::with_capacity(self.expressions.len()); - let mut new_indexes = IndexVec::from_elem_n(None, self.expressions.len()); - - // This closure converts any `Expression` operand (`lhs` or `rhs` of the `Op::Add` or - // `Op::Subtract` operation) into its native `llvm::coverage::Counter::CounterKind` type - // and value. - // - // Expressions will be returned from this function in a sequential vector (array) of - // `CounterExpression`, so the expression IDs must be mapped from their original, - // potentially sparse set of indexes. - // - // An `Expression` as an operand will have already been encountered as an `Expression` with - // operands, so its new_index will already have been generated (as a 1-up index value). - // (If an `Expression` as an operand does not have a corresponding new_index, it was - // probably optimized out, after the expression was injected into the MIR, so it will - // get a `CounterKind::Zero` instead.) - // - // In other words, an `Expression`s at any given index can include other expressions as - // operands, but expression operands can only come from the subset of expressions having - // `expression_index`s lower than the referencing `Expression`. Therefore, it is - // reasonable to look up the new index of an expression operand while the `new_indexes` - // vector is only complete up to the current `ExpressionIndex`. - type NewIndexes = IndexSlice>; - let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { - Operand::Zero => Some(Counter::ZERO), - Operand::Counter(id) => Some(Counter::counter_value_reference(id)), - Operand::Expression(id) => { - self.expressions - .get(id) - .expect("expression id is out of range") - .as_ref() - // If an expression was optimized out, assume it would have produced a count - // of zero. This ensures that expressions dependent on optimized-out - // expressions are still valid. - .map_or(Some(Counter::ZERO), |_| new_indexes[id].map(Counter::expression)) - } - }; - - for (original_index, expression) in - self.expressions.iter_enumerated().filter_map(|(original_index, entry)| { - // Option::map() will return None to filter out missing expressions. This may happen - // if, for example, a MIR-instrumented expression is removed during an optimization. - entry.as_ref().map(|expression| (original_index, expression)) - }) - { - let optional_region = &expression.region; - let Expression { lhs, op, rhs, .. } = *expression; + /// Convert this function's coverage expression data into a form that can be + /// passed through FFI to LLVM. + fn counter_expressions(&self) -> Vec { + // 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 + // thing on the Rust side unless we're confident we can do much better. + // (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.) - if let Some(Some((lhs_counter, mut rhs_counter))) = id_to_counter(&new_indexes, lhs) - .map(|lhs_counter| { - id_to_counter(&new_indexes, rhs).map(|rhs_counter| (lhs_counter, rhs_counter)) - }) - { - if lhs_counter.is_zero() && op.is_subtract() { - // The left side of a subtraction was probably optimized out. As an example, - // a branch condition might be evaluated as a constant expression, and the - // branch could be removed, dropping unused counters in the process. - // - // Since counters are unsigned, we must assume the result of the expression - // can be no more and no less than zero. An expression known to evaluate to zero - // does not need to be added to the coverage map. - // - // Coverage test `loops_branches.rs` includes multiple variations of branches - // based on constant conditional (literal `true` or `false`), and demonstrates - // that the expected counts are still correct. - debug!( - "Expression subtracts from zero (assume unreachable): \ - original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", - original_index, lhs, op, rhs, optional_region, - ); - rhs_counter = Counter::ZERO; + self.expressions + .iter() + .map(|expression| match expression { + None => { + // This expression ID was allocated, but we never saw the + // 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 } - debug_assert!( - lhs_counter.is_zero() - // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` - || ((lhs_counter.zero_based_id() as usize) - <= usize::max(self.counters.len(), self.expressions.len())), - "lhs id={} > both counters.len()={} and expressions.len()={} - ({:?} {:?} {:?})", - lhs_counter.zero_based_id(), - self.counters.len(), - self.expressions.len(), - lhs_counter, - op, - rhs_counter, - ); - - debug_assert!( - rhs_counter.is_zero() - // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` - || ((rhs_counter.zero_based_id() as usize) - <= usize::max(self.counters.len(), self.expressions.len())), - "rhs id={} > both counters.len()={} and expressions.len()={} - ({:?} {:?} {:?})", - rhs_counter.zero_based_id(), - self.counters.len(), - self.expressions.len(), - lhs_counter, - op, - rhs_counter, - ); - - // Both operands exist. `Expression` operands exist in `self.expressions` and have - // been assigned a `new_index`. - let mapped_expression_index = - MappedExpressionIndex::from(counter_expressions.len()); - let expression = CounterExpression::new( - lhs_counter, - match op { - Op::Add => ExprKind::Add, - Op::Subtract => ExprKind::Subtract, - }, - rhs_counter, - ); - debug!( - "Adding expression {:?} = {:?}, region: {:?}", - mapped_expression_index, expression, optional_region - ); - counter_expressions.push(expression); - new_indexes[original_index] = Some(mapped_expression_index); - if let Some(region) = optional_region { - expression_regions.push((Counter::expression(mapped_expression_index), region)); + &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), + ) } - } else { - bug!( - "expression has one or more missing operands \ - original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", - original_index, - lhs, - op, - rhs, - optional_region, - ); - } - } - (counter_expressions, expression_regions.into_iter()) + }) + .collect::>() + } + + fn expression_regions(&self) -> Vec<(Counter, &CodeRegion)> { + // Find all of the expression IDs that weren't optimized out AND have + // an attached code region, and return the corresponding mapping as a + // counter/region pair. + self.expressions + .iter_enumerated() + .filter_map(|(id, expression)| { + let code_region = expression.as_ref()?.region.as_ref()?; + Some((Counter::expression(id), code_region)) + }) + .collect::>() } fn unreachable_regions(&self) -> impl Iterator { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 1efb54bdb0872..9ef6739229118 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -45,16 +45,6 @@ impl ExpressionId { } } -rustc_index::newtype_index! { - /// MappedExpressionIndex values ascend from zero, and are recalculated indexes based on their - /// array position in the LLVM coverage map "Expressions" array, which is assembled during the - /// "mapgen" process. They cannot be computed algorithmically, from the other `newtype_index`s. - #[derive(HashStable)] - #[max = 0xFFFF_FFFF] - #[debug_format = "MappedExpressionIndex({})"] - pub struct MappedExpressionIndex {} -} - /// Operand of a coverage-counter expression. /// /// Operands can be a constant zero value, an actual coverage counter, or another diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index f4158597d100c..5a12b6aebf111 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -478,7 +478,6 @@ TrivialTypeTraversalImpls! { ::rustc_target::asm::InlineAsmRegOrRegClass, crate::mir::coverage::CounterId, crate::mir::coverage::ExpressionId, - crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, crate::mir::Promoted, crate::traits::Reveal,