From 1a014d42f45de1b829ca7916d8f639fda6e0770a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 26 Jun 2023 22:28:48 +1000 Subject: Replace `ExpressionOperandId` with enum `Operand` Because the three kinds of operand are now distinguished explicitly, we no longer need fiddly code to disambiguate counter IDs and expression IDs based on the total number of counters/expressions in a function. This does increase the size of operands from 4 bytes to 8 bytes, but that shouldn't be a big deal since they are mostly stored inside boxed structures, and the current coverage code is not particularly size-optimized anyway. --- .../src/coverageinfo/map_data.rs | 43 ++++++++++------------ .../rustc_codegen_llvm/src/coverageinfo/mod.rs | 6 +-- 2 files changed, 22 insertions(+), 27 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 06844afd6b8..0d4d8ca61e9 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,17 +3,17 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, ExpressionOperandId, InjectedExpressionId, - InjectedExpressionIndex, MappedExpressionIndex, Op, + CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, + MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; #[derive(Clone, Debug, PartialEq)] pub struct Expression { - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, } @@ -105,16 +105,16 @@ impl<'tcx> FunctionCoverage<'tcx> { pub fn add_counter_expression( &mut self, expression_id: InjectedExpressionId, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, ) { debug!( "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", expression_id, lhs, op, rhs, region ); - let expression_index = self.expression_index(u32::from(expression_id)); + let expression_index = self.expression_index(expression_id); debug_assert!( expression_index.as_usize() < self.expressions.len(), "expression_index {} is out of range for expressions.len() = {} @@ -186,10 +186,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // 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. Operand ID value `0` maps to `CounterKind::Zero`; values in the known range - // of injected LLVM counters map to `CounterKind::CounterValueReference` (and the value - // matches the injected counter index); and any other value is converted into a - // `CounterKind::Expression` with the expression's `new_index`. + // 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, @@ -206,17 +203,13 @@ impl<'tcx> FunctionCoverage<'tcx> { // `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`. - let id_to_counter = |new_indexes: &IndexSlice< - InjectedExpressionIndex, - Option, - >, - id: ExpressionOperandId| { - if id == ExpressionOperandId::ZERO { - Some(Counter::zero()) - } else if id.index() < self.counters.len() { + type NewIndexes = IndexSlice>; + let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { + Operand::Zero => Some(Counter::zero()), + Operand::Counter(id) => { debug_assert!( id.index() > 0, - "ExpressionOperandId indexes for counters are 1-based, but this id={}", + "Operand::Counter indexes for counters are 1-based, but this id={}", id.index() ); // Note: Some codegen-injected Counters may be only referenced by `Expression`s, @@ -224,8 +217,9 @@ impl<'tcx> FunctionCoverage<'tcx> { let index = CounterValueReference::from(id.index()); // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based. Some(Counter::counter_value_reference(index)) - } else { - let index = self.expression_index(u32::from(id)); + } + Operand::Expression(id) => { + let index = self.expression_index(id); self.expressions .get(index) .expect("expression id is out of range") @@ -341,8 +335,9 @@ impl<'tcx> FunctionCoverage<'tcx> { self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) } - fn expression_index(&self, id_descending_from_max: u32) -> InjectedExpressionIndex { - debug_assert!(id_descending_from_max >= self.counters.len() as u32); + fn expression_index(&self, id: InjectedExpressionId) -> InjectedExpressionIndex { + debug_assert!(id.as_usize() >= self.counters.len()); + let id_descending_from_max = id.as_u32(); InjectedExpressionIndex::from(u32::MAX - id_descending_from_max) } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 60cbb3d3d9d..d6a32497f7f 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, ExpressionOperandId, InjectedExpressionId, Op, + CodeRegion, CounterValueReference, CoverageKind, InjectedExpressionId, Op, Operand, }; use rustc_middle::mir::Coverage; use rustc_middle::ty; @@ -203,9 +203,9 @@ impl<'tcx> Builder<'_, '_, 'tcx> { &mut self, instance: Instance<'tcx>, id: InjectedExpressionId, - lhs: ExpressionOperandId, + lhs: Operand, op: Op, - rhs: ExpressionOperandId, + rhs: Operand, region: Option, ) -> bool { if let Some(coverage_context) = self.coverage_context() { -- cgit 1.4.1-3-g733a5 From f103db894fdcf94822d57cf28e30bc498c042631 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 29 Jun 2023 12:14:04 +1000 Subject: Make coverage expression IDs count up from 0, not down from `u32::MAX` Operand types are now tracked explicitly, so there is no need for expression IDs to avoid counter IDs by descending from `u32::MAX`. Instead they can just count up from 0, and can be used directly as indices when necessary. --- .../src/coverageinfo/map_data.rs | 49 +++++++--------------- .../rustc_codegen_llvm/src/coverageinfo/mod.rs | 4 +- compiler/rustc_middle/src/mir/coverage.rs | 26 +++++++----- compiler/rustc_middle/src/ty/structural_impls.rs | 3 +- .../rustc_mir_transform/src/coverage/counters.rs | 19 ++++----- compiler/rustc_mir_transform/src/coverage/debug.rs | 2 +- compiler/rustc_mir_transform/src/coverage/query.rs | 9 ++-- ...coverage_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...nstrument_coverage.main.InstrumentCoverage.diff | 6 +-- 9 files changed, 49 insertions(+), 71 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 0d4d8ca61e9..4fada329367 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,8 +3,7 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, - MappedExpressionIndex, Op, Operand, + CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -19,8 +18,7 @@ pub struct Expression { /// Collects all of the coverage regions associated with (a) injected counters, (b) counter /// expressions (additions or subtraction), and (c) unreachable regions (always counted as zero), -/// for a given Function. Counters and counter expressions have non-overlapping `id`s because they -/// can both be operands in an expression. This struct also stores the `function_source_hash`, +/// for a given Function. This struct also stores the `function_source_hash`, /// computed during instrumentation, and forwarded with counters. /// /// Note, it may be important to understand LLVM's definitions of `unreachable` regions versus "gap @@ -35,7 +33,7 @@ pub struct FunctionCoverage<'tcx> { source_hash: u64, is_used: bool, counters: IndexVec>, - expressions: IndexVec>, + expressions: IndexVec>, unreachable_regions: Vec, } @@ -89,22 +87,11 @@ impl<'tcx> FunctionCoverage<'tcx> { } /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other - /// expressions. Expression IDs start from `u32::MAX` and go down, so the range of expression - /// IDs will not overlap with the range of counter IDs. Counters and expressions can be added in - /// any order, and expressions can still be assigned contiguous (though descending) IDs, without - /// knowing what the last counter ID will be. - /// - /// When storing the expression data in the `expressions` vector in the `FunctionCoverage` - /// struct, its vector index is computed, from the given expression ID, by subtracting from - /// `u32::MAX`. - /// - /// Since the expression operands (`lhs` and `rhs`) can reference either counters or - /// expressions, an operand that references an expression also uses its original ID, descending - /// from `u32::MAX`. Theses operands are translated only during code generation, after all - /// counters and expressions have been added. + /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity + /// between operands that are counter IDs and operands that are expression IDs. pub fn add_counter_expression( &mut self, - expression_id: InjectedExpressionId, + expression_id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, @@ -114,16 +101,15 @@ impl<'tcx> FunctionCoverage<'tcx> { "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", expression_id, lhs, op, rhs, region ); - let expression_index = self.expression_index(expression_id); debug_assert!( - expression_index.as_usize() < self.expressions.len(), - "expression_index {} is out of range for expressions.len() = {} + expression_id.as_usize() < self.expressions.len(), + "expression_id {} is out of range for expressions.len() = {} for {:?}", - expression_index.as_usize(), + expression_id.as_usize(), self.expressions.len(), self, ); - if let Some(previous_expression) = self.expressions[expression_index].replace(Expression { + if let Some(previous_expression) = self.expressions[expression_id].replace(Expression { lhs, op, rhs, @@ -190,7 +176,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // // 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, originally in reverse order from `u32::MAX`. + // 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). @@ -203,7 +189,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // `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>; + type NewIndexes = IndexSlice>; let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { Operand::Zero => Some(Counter::zero()), Operand::Counter(id) => { @@ -219,15 +205,14 @@ impl<'tcx> FunctionCoverage<'tcx> { Some(Counter::counter_value_reference(index)) } Operand::Expression(id) => { - let index = self.expression_index(id); self.expressions - .get(index) + .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[index].map(Counter::expression)) + .map_or(Some(Counter::zero()), |_| new_indexes[id].map(Counter::expression)) } }; @@ -334,10 +319,4 @@ impl<'tcx> FunctionCoverage<'tcx> { fn unreachable_regions(&self) -> impl Iterator { self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) } - - fn expression_index(&self, id: InjectedExpressionId) -> InjectedExpressionIndex { - debug_assert!(id.as_usize() >= self.counters.len()); - let id_descending_from_max = id.as_u32(); - InjectedExpressionIndex::from(u32::MAX - id_descending_from_max) - } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index d6a32497f7f..5e0c5df2bcb 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, InjectedExpressionId, Op, Operand, + CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand, }; use rustc_middle::mir::Coverage; use rustc_middle::ty; @@ -202,7 +202,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> { fn add_coverage_counter_expression( &mut self, instance: Instance<'tcx>, - id: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 7894e564b02..bcec46d9149 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -26,19 +26,23 @@ impl CounterValueReference { } rustc_index::newtype_index! { - /// Values descend from u32::MAX. + /// ID of a coverage-counter expression. Values ascend from 0. + /// + /// Note that LLVM handles expression IDs as `uint32_t`, so there is no need + /// to use a larger representation on the Rust side. #[derive(HashStable)] #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionId({})"] - pub struct InjectedExpressionId {} + #[debug_format = "ExpressionId({})"] + pub struct ExpressionId {} } -rustc_index::newtype_index! { - /// Values ascend from 0. - #[derive(HashStable)] - #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionIndex({})"] - pub struct InjectedExpressionIndex {} +impl ExpressionId { + pub const START: Self = Self::from_u32(0); + + #[inline(always)] + pub fn next_id(self) -> Self { + Self::from_u32(self.as_u32() + 1) + } } rustc_index::newtype_index! { @@ -60,7 +64,7 @@ rustc_index::newtype_index! { pub enum Operand { Zero, Counter(CounterValueReference), - Expression(InjectedExpressionId), + Expression(ExpressionId), } impl Debug for Operand { @@ -84,7 +88,7 @@ pub enum CoverageKind { Expression { /// ID of this coverage-counter expression within its enclosing function. /// Other expressions in the same function can refer to it as an operand. - id: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 10cff0e2f50..15cf4c46788 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -471,8 +471,7 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, crate::mir::coverage::CounterValueReference, - crate::mir::coverage::InjectedExpressionId, - crate::mir::coverage::InjectedExpressionIndex, + crate::mir::coverage::ExpressionId, crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, crate::mir::Promoted, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 9d168404264..cd2539a5e66 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -17,7 +17,7 @@ use rustc_middle::mir::coverage::*; pub(super) struct CoverageCounters { function_source_hash: u64, next_counter_id: u32, - num_expressions: u32, + next_expression_id: ExpressionId, pub debug_counters: DebugCounters, } @@ -26,7 +26,7 @@ impl CoverageCounters { Self { function_source_hash, next_counter_id: CounterValueReference::START.as_u32(), - num_expressions: 0, + next_expression_id: ExpressionId::START, debug_counters: DebugCounters::new(), } } @@ -94,20 +94,17 @@ impl CoverageCounters { /// Counter IDs start from one and go up. fn next_counter(&mut self) -> CounterValueReference { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); let next = self.next_counter_id; self.next_counter_id += 1; CounterValueReference::from(next) } - /// Expression IDs start from u32::MAX and go down because an Expression can reference - /// (add or subtract counts) of both Counter regions and Expression regions. The counter - /// expression operand IDs must be unique across both types. - fn next_expression(&mut self) -> InjectedExpressionId { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); - let next = u32::MAX - self.num_expressions; - self.num_expressions += 1; - InjectedExpressionId::from(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 = next.next_id(); + next } } diff --git a/compiler/rustc_mir_transform/src/coverage/debug.rs b/compiler/rustc_mir_transform/src/coverage/debug.rs index 0e1065a40e7..26f9cfd0b86 100644 --- a/compiler/rustc_mir_transform/src/coverage/debug.rs +++ b/compiler/rustc_mir_transform/src/coverage/debug.rs @@ -485,7 +485,7 @@ impl GraphvizData { /// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs /// and/or a `CoverageGraph` graphviz output). pub(super) struct UsedExpressions { - some_used_expression_operands: Option>>, + some_used_expression_operands: Option>>, some_unused_expressions: Option, BasicCoverageBlock)>>, } diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 1261cf2765c..c1a896dbd34 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -52,12 +52,11 @@ impl CoverageVisitor { self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } - /// Computes an expression index for each expression ID, and updates `num_expressions` to the - /// maximum encountered index plus 1. + /// Updates `num_expressions` to the maximum encountered expression ID plus 1. #[inline(always)] - fn update_num_expressions(&mut self, expression_id: InjectedExpressionId) { - let expression_index = u32::MAX - expression_id.as_u32(); - self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1); + fn update_num_expressions(&mut self, expression_id: ExpressionId) { + let expression_id = expression_id.as_u32(); + self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_id + 1); } fn update_from_expression_operand(&mut self, operand: Operand) { diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index 5d19b863114..affd0fb18d5 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -3,7 +3,7 @@ digraph Cov_0_3 { node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; - bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(4294967293) = Expression(4294967294) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; + bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>]; diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 5bd271456c6..e87ed5268e0 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -13,7 +13,7 @@ } bb1: { -+ Coverage::Expression(4294967295) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,8 +27,8 @@ } bb4: { -+ Coverage::Expression(4294967293) = Expression(4294967294) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(4294967294) = Expression(4294967295) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; ++ Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return; -- cgit 1.4.1-3-g733a5 From 3920e07f0bd97d9815a037eaeea197266424cd56 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 29 Jun 2023 12:36:19 +1000 Subject: Make coverage counter IDs count up from 0, not 1 Operand types are now tracked explicitly, so there is no need to reserve ID 0 for the special always-zero counter. As part of the renumbering, this change fixes an off-by-one error in the way counters were counted by the `coverageinfo` query. As a result, functions should now have exactly the number of counters they actually need, instead of always having an extra counter that is never used. --- .../rustc_codegen_llvm/src/coverageinfo/ffi.rs | 10 ++++----- .../src/coverageinfo/map_data.rs | 19 ++++------------ .../rustc_codegen_llvm/src/coverageinfo/mod.rs | 10 ++++----- compiler/rustc_middle/src/mir/coverage.rs | 26 +++++++++++----------- compiler/rustc_middle/src/ty/structural_impls.rs | 2 +- .../rustc_mir_transform/src/coverage/counters.rs | 10 ++++----- compiler/rustc_mir_transform/src/coverage/query.rs | 9 +++----- compiler/rustc_mir_transform/src/coverage/tests.rs | 4 ++-- .../coverage_graphviz.bar.InstrumentCoverage.0.dot | 2 +- ...coverage_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...instrument_coverage.bar.InstrumentCoverage.diff | 2 +- ...nstrument_coverage.main.InstrumentCoverage.diff | 8 +++---- 12 files changed, 43 insertions(+), 61 deletions(-) (limited to 'compiler/rustc_codegen_llvm/src') diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 1791ce4b315..6ca61aa43d7 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::{CounterValueReference, MappedExpressionIndex}; +use rustc_middle::mir::coverage::{CounterId, MappedExpressionIndex}; /// Must match the layout of `LLVMRustCounterKind`. #[derive(Copy, Clone, Debug)] @@ -36,11 +36,9 @@ impl Counter { Self { kind: CounterKind::Zero, id: 0 } } - /// Constructs a new `Counter` of kind `CounterValueReference`, and converts - /// the given 1-based counter_id to the required 0-based equivalent for - /// the `Counter` encoding. - pub fn counter_value_reference(counter_id: CounterValueReference) -> Self { - Self { kind: CounterKind::CounterValueReference, id: counter_id.zero_based_index() } + /// Constructs a new `Counter` of kind `CounterValueReference`. + pub fn counter_value_reference(counter_id: CounterId) -> Self { + Self { kind: CounterKind::CounterValueReference, id: counter_id.as_u32() } } /// Constructs a new `Counter` of kind `Expression`. diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 4fada329367..7e981af0a53 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,7 +3,7 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand, + CodeRegion, CounterId, ExpressionId, MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -32,7 +32,7 @@ pub struct FunctionCoverage<'tcx> { instance: Instance<'tcx>, source_hash: u64, is_used: bool, - counters: IndexVec>, + counters: IndexVec>, expressions: IndexVec>, unreachable_regions: Vec, } @@ -80,7 +80,7 @@ impl<'tcx> FunctionCoverage<'tcx> { } /// Adds a code region to be counted by an injected counter intrinsic. - pub fn add_counter(&mut self, id: CounterValueReference, region: CodeRegion) { + pub fn add_counter(&mut self, id: CounterId, region: CodeRegion) { if let Some(previous_region) = self.counters[id].replace(region.clone()) { assert_eq!(previous_region, region, "add_counter: code region for id changed"); } @@ -192,18 +192,7 @@ impl<'tcx> FunctionCoverage<'tcx> { type NewIndexes = IndexSlice>; let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { Operand::Zero => Some(Counter::zero()), - Operand::Counter(id) => { - debug_assert!( - id.index() > 0, - "Operand::Counter indexes for counters are 1-based, but this id={}", - id.index() - ); - // Note: Some codegen-injected Counters may be only referenced by `Expression`s, - // and may not have their own `CodeRegion`s, - let index = CounterValueReference::from(id.index()); - // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based. - Some(Counter::counter_value_reference(index)) - } + Operand::Counter(id) => Some(Counter::counter_value_reference(id)), Operand::Expression(id) => { self.expressions .get(id) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 5e0c5df2bcb..c1017ab8d7c 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -16,9 +16,7 @@ use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; -use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand, -}; +use rustc_middle::mir::coverage::{CodeRegion, CounterId, CoverageKind, ExpressionId, Op, Operand}; use rustc_middle::mir::Coverage; use rustc_middle::ty; use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt}; @@ -33,7 +31,7 @@ mod ffi; pub(crate) mod map_data; pub mod mapgen; -const UNUSED_FUNCTION_COUNTER_ID: CounterValueReference = CounterValueReference::START; +const UNUSED_FUNCTION_COUNTER_ID: CounterId = CounterId::START; const VAR_ALIGN_BYTES: usize = 8; @@ -125,7 +123,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { let fn_name = bx.get_pgo_func_name_var(instance); let hash = bx.const_u64(function_source_hash); let num_counters = bx.const_u32(coverageinfo.num_counters); - let index = bx.const_u32(id.zero_based_index()); + let index = bx.const_u32(id.as_u32()); debug!( "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})", fn_name, hash, num_counters, index, @@ -178,7 +176,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> { fn add_coverage_counter( &mut self, instance: Instance<'tcx>, - id: CounterValueReference, + id: CounterId, region: CodeRegion, ) -> bool { if let Some(coverage_context) = self.coverage_context() { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index bcec46d9149..d7d6e3a0086 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -6,22 +6,22 @@ use rustc_span::Symbol; use std::fmt::{self, Debug, Formatter}; rustc_index::newtype_index! { + /// ID of a coverage counter. Values ascend from 0. + /// + /// Note that LLVM handles counter IDs as `uint32_t`, so there is no need + /// to use a larger representation on the Rust side. #[derive(HashStable)] #[max = 0xFFFF_FFFF] - #[debug_format = "CounterValueReference({})"] - pub struct CounterValueReference {} + #[debug_format = "CounterId({})"] + pub struct CounterId {} } -impl CounterValueReference { - /// Counters start at 1 for historical reasons. - pub const START: Self = Self::from_u32(1); +impl CounterId { + pub const START: Self = Self::from_u32(0); - /// Returns explicitly-requested zero-based version of the counter id, used - /// during codegen. LLVM expects zero-based indexes. - pub fn zero_based_index(self) -> u32 { - let one_based_index = self.as_u32(); - debug_assert!(one_based_index > 0); - one_based_index - 1 + #[inline(always)] + pub fn next_id(self) -> Self { + Self::from_u32(self.as_u32() + 1) } } @@ -63,7 +63,7 @@ rustc_index::newtype_index! { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub enum Operand { Zero, - Counter(CounterValueReference), + Counter(CounterId), Expression(ExpressionId), } @@ -83,7 +83,7 @@ pub enum CoverageKind { function_source_hash: u64, /// ID of this counter within its enclosing function. /// Expressions in the same function can refer to it as an operand. - id: CounterValueReference, + id: CounterId, }, Expression { /// ID of this coverage-counter expression within its enclosing function. diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 15cf4c46788..c6b6f0e8990 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -470,7 +470,7 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_hir::Unsafety, ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, - crate::mir::coverage::CounterValueReference, + crate::mir::coverage::CounterId, crate::mir::coverage::ExpressionId, crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index cd2539a5e66..97bdb878ab1 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -16,7 +16,7 @@ use rustc_middle::mir::coverage::*; /// `Coverage` statements. pub(super) struct CoverageCounters { function_source_hash: u64, - next_counter_id: u32, + next_counter_id: CounterId, next_expression_id: ExpressionId, pub debug_counters: DebugCounters, } @@ -25,7 +25,7 @@ impl CoverageCounters { pub fn new(function_source_hash: u64) -> Self { Self { function_source_hash, - next_counter_id: CounterValueReference::START.as_u32(), + next_counter_id: CounterId::START, next_expression_id: ExpressionId::START, debug_counters: DebugCounters::new(), } @@ -93,10 +93,10 @@ impl CoverageCounters { } /// Counter IDs start from one and go up. - fn next_counter(&mut self) -> CounterValueReference { + fn next_counter(&mut self) -> CounterId { let next = self.next_counter_id; - self.next_counter_id += 1; - CounterValueReference::from(next) + self.next_counter_id = next.next_id(); + next } /// Expression IDs start from 0 and go up. diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index c1a896dbd34..aa205655f9d 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -43,11 +43,9 @@ struct CoverageVisitor { } impl CoverageVisitor { - /// Updates `num_counters` to the maximum encountered zero-based counter_id plus 1. Note the - /// final computed number of counters should be the number of all `CoverageKind::Counter` - /// statements in the MIR *plus one* for the implicit `ZERO` counter. + /// Updates `num_counters` to the maximum encountered counter ID plus 1. #[inline(always)] - fn update_num_counters(&mut self, counter_id: CounterValueReference) { + fn update_num_counters(&mut self, counter_id: CounterId) { let counter_id = counter_id.as_u32(); self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } @@ -103,8 +101,7 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> let mir_body = tcx.instance_mir(instance_def); let mut coverage_visitor = CoverageVisitor { - // num_counters always has at least the `ZERO` counter. - info: CoverageInfo { num_counters: 1, num_expressions: 0 }, + info: CoverageInfo { num_counters: 0, num_expressions: 0 }, add_missing_operands: false, }; diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index 25891d3ca0f..248a192f8f5 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -683,7 +683,7 @@ fn test_make_bcb_counters() { let_bcb!(1); assert_eq!( - 1, // coincidentally, bcb1 has a `Counter` with id = 1 + 0, // bcb1 has a `Counter` with id = 0 match basic_coverage_blocks[bcb1].counter().expect("should have a counter") { CoverageKind::Counter { id, .. } => id, _ => panic!("expected a Counter"), @@ -693,7 +693,7 @@ fn test_make_bcb_counters() { let_bcb!(2); assert_eq!( - 2, // coincidentally, bcb2 has a `Counter` with id = 2 + 1, // bcb2 has a `Counter` with id = 1 match basic_coverage_blocks[bcb2].counter().expect("should have a counter") { CoverageKind::Counter { id, .. } => id, _ => panic!("expected a Counter"), diff --git a/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot index 03df5c9504b..3b90aaeae42 100644 --- a/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.bar.InstrumentCoverage.0.dot @@ -2,5 +2,5 @@ digraph Cov_0_4 { graph [fontname="Courier, monospace"]; node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; - bcb0__Cov_0_4 [shape="none", label=<
bcb0
Counter(bcb0) at 18:1-20:2
19:5-19:9: @0[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:18:1 - 20:2
20:2-20:2: @0.Return: return
bb0: Return
>]; + bcb0__Cov_0_4 [shape="none", label=<
bcb0
Counter(bcb0) at 18:1-20:2
19:5-19:9: @0[0]: Coverage::Counter(0) for $DIR/coverage_graphviz.rs:18:1 - 20:2
20:2-20:2: @0.Return: return
bb0: Return
>]; } diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index affd0fb18d5..19c220e2e1d 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -2,7 +2,7 @@ digraph Cov_0_3 { graph [fontname="Courier, monospace"]; node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; - bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; + bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff index 0aece766bd5..afcfde09c02 100644 --- a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff @@ -5,7 +5,7 @@ let mut _0: bool; bb0: { -+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:20:1 - 22:2; ++ Coverage::Counter(0) for /the/src/instrument_coverage.rs:20:1 - 22:2; _0 = const true; return; } diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index e87ed5268e0..e17c6ddc56e 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -8,12 +8,12 @@ let mut _3: !; bb0: { -+ Coverage::Counter(1) for /the/src/instrument_coverage.rs:11:1 - 11:11; ++ Coverage::Counter(0) for /the/src/instrument_coverage.rs:11:1 - 11:11; goto -> bb1; } bb1: { -+ Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(0) = Counter(0) + Counter(1) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -28,14 +28,14 @@ bb4: { + Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(1) = Expression(0) - Counter(1) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return; } bb5: { -+ Coverage::Counter(2) for /the/src/instrument_coverage.rs:15:10 - 15:11; ++ Coverage::Counter(1) for /the/src/instrument_coverage.rs:15:10 - 15:11; _1 = const (); StorageDead(_2); goto -> bb1; -- cgit 1.4.1-3-g733a5