From 3f549466a84754f25b1daf28c315eb97b548f218 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 30 Aug 2023 23:29:37 +1000 Subject: [PATCH 1/3] coverage: Extract a common iterator over a function's coverage statements Both of the coverage queries can now use this one helper function to iterate over all of the `mir::Coverage` payloads in the statements of a `mir::Body`. --- .../rustc_mir_transform/src/coverage/query.rs | 42 +++++++++---------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index aa205655f9dab..9bf9932eab25e 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -1,5 +1,6 @@ use super::*; +use rustc_data_structures::captures::Captures; use rustc_middle::mir::coverage::*; use rustc_middle::mir::{self, Body, Coverage, CoverageInfo}; use rustc_middle::query::Providers; @@ -66,15 +67,8 @@ impl CoverageVisitor { } fn visit_body(&mut self, body: &Body<'_>) { - for bb_data in body.basic_blocks.iter() { - for statement in bb_data.statements.iter() { - if let StatementKind::Coverage(box ref coverage) = statement.kind { - if is_inlined(body, statement) { - continue; - } - self.visit_coverage(coverage); - } - } + for coverage in all_coverage_in_mir_body(body) { + self.visit_coverage(coverage); } } @@ -115,23 +109,25 @@ fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> { let body = mir_body(tcx, def_id); - body.basic_blocks - .iter() - .flat_map(|data| { - data.statements.iter().filter_map(|statement| match statement.kind { - StatementKind::Coverage(box ref coverage) => { - if is_inlined(body, statement) { - None - } else { - coverage.code_region.as_ref() // may be None - } - } - _ => None, - }) - }) + all_coverage_in_mir_body(body) + // Not all coverage statements have an attached code region. + .filter_map(|coverage| coverage.code_region.as_ref()) .collect() } +fn all_coverage_in_mir_body<'a, 'tcx>( + body: &'a Body<'tcx>, +) -> impl Iterator + Captures<'tcx> { + body.basic_blocks.iter().flat_map(|bb_data| &bb_data.statements).filter_map(|statement| { + match statement.kind { + StatementKind::Coverage(box ref coverage) if !is_inlined(body, statement) => { + Some(coverage) + } + _ => None, + } + }) +} + 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() From f191b1c2fc2bbd5585e3c436d2deaae203345c5e Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 30 Aug 2023 23:36:11 +1000 Subject: [PATCH 2/3] coverage: Simplify the `coverageinfo` query to a single pass --- .../rustc_mir_transform/src/coverage/query.rs | 43 ++++++------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 9bf9932eab25e..90efe02cb2544 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -29,18 +29,12 @@ pub(crate) fn provide(providers: &mut Providers) { /// calls may not work; but computing the number of counters or expressions by adding `1` to the /// highest ID (for a given instrumented function) is valid. /// -/// This visitor runs twice, first with `add_missing_operands` set to `false`, to find the maximum -/// counter ID and maximum expression ID based on their enum variant `id` fields; then, as a -/// safeguard, with `add_missing_operands` set to `true`, to find any other counter or expression -/// IDs referenced by expression operands, if not already seen. -/// -/// Ideally, each operand ID in a MIR `CoverageKind::Expression` will have a separate MIR `Coverage` -/// statement for the `Counter` or `Expression` with the referenced ID. but since current or future -/// MIR optimizations can theoretically optimize out segments of a MIR, it may not be possible to -/// guarantee this, so the second pass ensures the `CoverageInfo` counts include all referenced IDs. +/// It's possible for a coverage expression to remain in MIR while one or both of its operands +/// have been optimized away. To avoid problems in codegen, we include those operands' IDs when +/// determining the maximum counter/expression ID, even if the underlying counter/expression is +/// no longer present. struct CoverageVisitor { info: CoverageInfo, - add_missing_operands: bool, } impl CoverageVisitor { @@ -73,20 +67,14 @@ impl CoverageVisitor { } fn visit_coverage(&mut self, coverage: &Coverage) { - if self.add_missing_operands { - match coverage.kind { - CoverageKind::Expression { lhs, rhs, .. } => { - self.update_from_expression_operand(lhs); - self.update_from_expression_operand(rhs); - } - _ => {} - } - } else { - match coverage.kind { - CoverageKind::Counter { id, .. } => self.update_num_counters(id), - CoverageKind::Expression { id, .. } => self.update_num_expressions(id), - _ => {} + match coverage.kind { + CoverageKind::Counter { id, .. } => self.update_num_counters(id), + CoverageKind::Expression { id, lhs, rhs, .. } => { + self.update_num_expressions(id); + self.update_from_expression_operand(lhs); + self.update_from_expression_operand(rhs); } + CoverageKind::Unreachable => {} } } } @@ -94,14 +82,9 @@ impl CoverageVisitor { fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> CoverageInfo { let mir_body = tcx.instance_mir(instance_def); - let mut coverage_visitor = CoverageVisitor { - info: CoverageInfo { num_counters: 0, num_expressions: 0 }, - add_missing_operands: false, - }; - - coverage_visitor.visit_body(mir_body); + let mut coverage_visitor = + CoverageVisitor { info: CoverageInfo { num_counters: 0, num_expressions: 0 } }; - coverage_visitor.add_missing_operands = true; coverage_visitor.visit_body(mir_body); coverage_visitor.info From e54204c8e914959e8b42a7bd3531706e4a436fd6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 6 Sep 2023 12:37:38 +1000 Subject: [PATCH 3/3] coverage: In the visitor, track max counter/expression IDs without +1 This makes the visitor track the highest seen counter/expression IDs directly, and only add +1 (to convert to a vector length) at the very end. --- .../rustc_mir_transform/src/coverage/query.rs | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 90efe02cb2544..56365c5d474a7 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -13,15 +13,10 @@ pub(crate) fn provide(providers: &mut Providers) { providers.covered_code_regions = |tcx, def_id| covered_code_regions(tcx, def_id); } -/// The `num_counters` argument to `llvm.instrprof.increment` is the max counter_id + 1, or in -/// other words, the number of counter value references injected into the MIR (plus 1 for the -/// reserved `ZERO` counter, which uses counter ID `0` when included in an expression). Injected -/// counters have a counter ID from `1..num_counters-1`. -/// -/// `num_expressions` is the number of counter expressions added to the MIR body. -/// -/// Both `num_counters` and `num_expressions` are used to initialize new vectors, during backend -/// code generate, to lookup counters and expressions by simple u32 indexes. +/// Coverage codegen needs to know the total number of counter IDs and expression IDs that have +/// been used by a function's coverage mappings. These totals are used to create vectors to hold +/// the relevant counter and expression data, and the maximum counter ID (+ 1) is also needed by +/// the `llvm.instrprof.increment` intrinsic. /// /// MIR optimization may split and duplicate some BasicBlock sequences, or optimize out some code /// including injected counters. (It is OK if some counters are optimized out, but those counters @@ -34,28 +29,27 @@ pub(crate) fn provide(providers: &mut Providers) { /// determining the maximum counter/expression ID, even if the underlying counter/expression is /// no longer present. struct CoverageVisitor { - info: CoverageInfo, + max_counter_id: CounterId, + max_expression_id: ExpressionId, } impl CoverageVisitor { - /// Updates `num_counters` to the maximum encountered counter ID plus 1. + /// Updates `max_counter_id` to the maximum encountered counter ID. #[inline(always)] - 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); + fn update_max_counter_id(&mut self, counter_id: CounterId) { + self.max_counter_id = self.max_counter_id.max(counter_id); } - /// Updates `num_expressions` to the maximum encountered expression ID plus 1. + /// Updates `max_expression_id` to the maximum encountered expression ID. #[inline(always)] - 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_max_expression_id(&mut self, expression_id: ExpressionId) { + self.max_expression_id = self.max_expression_id.max(expression_id); } fn update_from_expression_operand(&mut self, operand: Operand) { match operand { - Operand::Counter(id) => self.update_num_counters(id), - Operand::Expression(id) => self.update_num_expressions(id), + Operand::Counter(id) => self.update_max_counter_id(id), + Operand::Expression(id) => self.update_max_expression_id(id), Operand::Zero => {} } } @@ -68,9 +62,9 @@ impl CoverageVisitor { fn visit_coverage(&mut self, coverage: &Coverage) { match coverage.kind { - CoverageKind::Counter { id, .. } => self.update_num_counters(id), + CoverageKind::Counter { id, .. } => self.update_max_counter_id(id), CoverageKind::Expression { id, lhs, rhs, .. } => { - self.update_num_expressions(id); + self.update_max_expression_id(id); self.update_from_expression_operand(lhs); self.update_from_expression_operand(rhs); } @@ -82,12 +76,18 @@ impl CoverageVisitor { fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> CoverageInfo { let mir_body = tcx.instance_mir(instance_def); - let mut coverage_visitor = - CoverageVisitor { info: CoverageInfo { num_counters: 0, num_expressions: 0 } }; + let mut coverage_visitor = CoverageVisitor { + max_counter_id: CounterId::START, + max_expression_id: ExpressionId::START, + }; coverage_visitor.visit_body(mir_body); - coverage_visitor.info + // Add 1 to the highest IDs to get the total number of IDs. + CoverageInfo { + num_counters: (coverage_visitor.max_counter_id + 1).as_u32(), + num_expressions: (coverage_visitor.max_expression_id + 1).as_u32(), + } } fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> {