Skip to content

Commit 20d051e

Browse files
committed
coverage: Defer part of counter-creation until codegen
1 parent ee7dc06 commit 20d051e

19 files changed

+196
-296
lines changed

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

+4-9
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ pub(crate) fn prepare_covfun_record<'tcx>(
5454
let fn_cov_info = tcx.instance_mir(instance.def).function_coverage_info.as_deref()?;
5555
let ids_info = tcx.coverage_ids_info(instance.def)?;
5656

57-
let expressions = prepare_expressions(fn_cov_info, ids_info, is_used);
57+
let expressions = prepare_expressions(ids_info, is_used);
5858

5959
let mut covfun = CovfunRecord {
6060
mangled_function_name: tcx.symbol_name(instance).name,
@@ -76,11 +76,7 @@ pub(crate) fn prepare_covfun_record<'tcx>(
7676
}
7777

7878
/// Convert the function's coverage-counter expressions into a form suitable for FFI.
79-
fn prepare_expressions(
80-
fn_cov_info: &FunctionCoverageInfo,
81-
ids_info: &CoverageIdsInfo,
82-
is_used: bool,
83-
) -> Vec<ffi::CounterExpression> {
79+
fn prepare_expressions(ids_info: &CoverageIdsInfo, is_used: bool) -> Vec<ffi::CounterExpression> {
8480
// If any counters or expressions were removed by MIR opts, replace their
8581
// terms with zero.
8682
let counter_for_term = |term| {
@@ -95,7 +91,7 @@ fn prepare_expressions(
9591
// producing the final coverage map, so there's no need to do the same
9692
// thing on the Rust side unless we're confident we can do much better.
9793
// (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.)
98-
fn_cov_info
94+
ids_info
9995
.expressions
10096
.iter()
10197
.map(move |&Expression { lhs, op, rhs }| ffi::CounterExpression {
@@ -142,8 +138,7 @@ fn fill_region_tables<'tcx>(
142138
// If the mapping refers to counters/expressions that were removed by
143139
// MIR opts, replace those occurrences with zero.
144140
let counter_for_bcb = |bcb: BasicCoverageBlock| -> ffi::Counter {
145-
let term =
146-
fn_cov_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term");
141+
let term = ids_info.term_for_bcb[bcb].expect("every BCB in a mapping was given a term");
147142
let term = if is_zero_term(term) { CovTerm::Zero } else { term };
148143
ffi::Counter::from_term(term)
149144
};

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

+5-14
Original file line numberDiff line numberDiff line change
@@ -160,17 +160,10 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
160160
CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!(
161161
"marker statement {kind:?} should have been removed by CleanupPostBorrowck"
162162
),
163-
CoverageKind::CounterIncrement { id } => {
164-
// The number of counters passed to `llvm.instrprof.increment` might
165-
// be smaller than the number originally inserted by the instrumentor,
166-
// if some high-numbered counters were removed by MIR optimizations.
167-
// If so, LLVM's profiler runtime will use fewer physical counters.
163+
CoverageKind::VirtualCounter { bcb }
164+
if let Some(&id) = ids_info.phys_counter_for_node.get(&bcb) =>
165+
{
168166
let num_counters = ids_info.num_counters_after_mir_opts();
169-
assert!(
170-
num_counters as usize <= function_coverage_info.num_counters,
171-
"num_counters disagreement: query says {num_counters} but function info only has {}",
172-
function_coverage_info.num_counters
173-
);
174167

175168
let fn_name = bx.get_pgo_func_name_var(instance);
176169
let hash = bx.const_u64(function_coverage_info.function_source_hash);
@@ -182,10 +175,8 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
182175
);
183176
bx.instrprof_increment(fn_name, hash, num_counters, index);
184177
}
185-
CoverageKind::ExpressionUsed { id: _ } => {
186-
// Expression-used statements are markers that are handled by
187-
// `coverage_ids_info`, so there's nothing to codegen here.
188-
}
178+
// If a BCB doesn't have an associated physical counter, there's nothing to codegen.
179+
CoverageKind::VirtualCounter { .. } => {}
189180
CoverageKind::CondBitmapUpdate { index, decision_depth } => {
190181
let cond_bitmap = coverage_cx
191182
.try_get_mcdc_condition_bitmap(&instance, decision_depth)

compiler/rustc_codegen_llvm/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#![feature(extern_types)]
1414
#![feature(file_buffered)]
1515
#![feature(hash_raw_entry)]
16+
#![feature(if_let_guard)]
1617
#![feature(impl_trait_in_assoc_type)]
1718
#![feature(iter_intersperse)]
1819
#![feature(let_chains)]

compiler/rustc_middle/src/mir/coverage.rs

+44-26
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@
22
33
use std::fmt::{self, Debug, Formatter};
44

5-
use rustc_index::IndexVec;
5+
use rustc_data_structures::fx::FxIndexMap;
66
use rustc_index::bit_set::DenseBitSet;
7+
use rustc_index::{Idx, IndexVec};
78
use rustc_macros::{HashStable, TyDecodable, TyEncodable};
89
use rustc_span::Span;
910

@@ -103,23 +104,12 @@ pub enum CoverageKind {
103104
/// Should be erased before codegen (at some point after `InstrumentCoverage`).
104105
BlockMarker { id: BlockMarkerId },
105106

106-
/// Marks the point in MIR control flow represented by a coverage counter.
107+
/// Marks its enclosing basic block with the ID of the coverage graph node
108+
/// that it was part of during the `InstrumentCoverage` MIR pass.
107109
///
108-
/// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR.
109-
///
110-
/// If this statement does not survive MIR optimizations, any mappings that
111-
/// refer to this counter can have those references simplified to zero.
112-
CounterIncrement { id: CounterId },
113-
114-
/// Marks the point in MIR control-flow represented by a coverage expression.
115-
///
116-
/// If this statement does not survive MIR optimizations, any mappings that
117-
/// refer to this expression can have those references simplified to zero.
118-
///
119-
/// (This is only inserted for expression IDs that are directly used by
120-
/// mappings. Intermediate expressions with no direct mappings are
121-
/// retained/zeroed based on whether they are transitively used.)
122-
ExpressionUsed { id: ExpressionId },
110+
/// During codegen, this might be lowered to `llvm.instrprof.increment` or
111+
/// to a no-op, depending on the outcome of counter-creation.
112+
VirtualCounter { bcb: BasicCoverageBlock },
123113

124114
/// Marks the point in MIR control flow represented by a evaluated condition.
125115
///
@@ -138,8 +128,7 @@ impl Debug for CoverageKind {
138128
match self {
139129
SpanMarker => write!(fmt, "SpanMarker"),
140130
BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()),
141-
CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()),
142-
ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()),
131+
VirtualCounter { bcb } => write!(fmt, "VirtualCounter({bcb:?})"),
143132
CondBitmapUpdate { index, decision_depth } => {
144133
write!(fmt, "CondBitmapUpdate(index={:?}, depth={:?})", index, decision_depth)
145134
}
@@ -208,11 +197,12 @@ pub struct FunctionCoverageInfo {
208197
pub function_source_hash: u64,
209198
pub body_span: Span,
210199

211-
pub num_counters: usize,
212-
pub expressions: IndexVec<ExpressionId, Expression>,
200+
/// Used in conjunction with `priority_list` to create physical counters
201+
/// and counter expressions, after MIR optimizations.
202+
pub node_flow_data: NodeFlowData<BasicCoverageBlock>,
203+
pub priority_list: Vec<BasicCoverageBlock>,
213204

214205
pub mappings: Vec<Mapping>,
215-
pub term_for_bcb: IndexVec<BasicCoverageBlock, Option<CovTerm>>,
216206

217207
pub mcdc_bitmap_bits: usize,
218208
/// The depth of the deepest decision is used to know how many
@@ -281,15 +271,18 @@ pub struct MCDCDecisionSpan {
281271
pub num_conditions: usize,
282272
}
283273

284-
/// Summarizes coverage IDs inserted by the `InstrumentCoverage` MIR pass
285-
/// (for compiler option `-Cinstrument-coverage`), after MIR optimizations
286-
/// have had a chance to potentially remove some of them.
274+
/// Contains information needed during codegen, obtained by inspecting the
275+
/// function's MIR after MIR optimizations.
287276
///
288-
/// Used by the `coverage_ids_info` query.
277+
/// Returned by the `coverage_ids_info` query.
289278
#[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable)]
290279
pub struct CoverageIdsInfo {
291280
pub counters_seen: DenseBitSet<CounterId>,
292281
pub zero_expressions: DenseBitSet<ExpressionId>,
282+
283+
pub phys_counter_for_node: FxIndexMap<BasicCoverageBlock, CounterId>,
284+
pub term_for_bcb: IndexVec<BasicCoverageBlock, Option<CovTerm>>,
285+
pub expressions: IndexVec<ExpressionId, Expression>,
293286
}
294287

295288
impl CoverageIdsInfo {
@@ -334,3 +327,28 @@ rustc_index::newtype_index! {
334327
const START_BCB = 0;
335328
}
336329
}
330+
331+
/// Data representing a view of some underlying graph, in which each node's
332+
/// successors have been merged into a single "supernode".
333+
///
334+
/// The resulting supernodes have no obvious meaning on their own.
335+
/// However, merging successor nodes means that a node's out-edges can all
336+
/// be combined into a single out-edge, whose flow is the same as the flow
337+
/// (execution count) of its corresponding node in the original graph.
338+
///
339+
/// With all node flows now in the original graph now represented as edge flows
340+
/// in the merged graph, it becomes possible to analyze the original node flows
341+
/// using techniques for analyzing edge flows.
342+
#[derive(Clone, Debug)]
343+
#[derive(TyEncodable, TyDecodable, Hash, HashStable)]
344+
pub struct NodeFlowData<Node: Idx> {
345+
/// Maps each node to the supernode that contains it, indicated by some
346+
/// arbitrary "root" node that is part of that supernode.
347+
pub supernodes: IndexVec<Node, Node>,
348+
/// For each node, stores the single supernode that all of its successors
349+
/// have been merged into.
350+
///
351+
/// (Note that each node in a supernode can potentially have a _different_
352+
/// successor supernode from its peers.)
353+
pub succ_supernodes: IndexVec<Node, Node>,
354+
}

compiler/rustc_middle/src/mir/pretty.rs

+1-5
Original file line numberDiff line numberDiff line change
@@ -619,13 +619,9 @@ fn write_function_coverage_info(
619619
function_coverage_info: &coverage::FunctionCoverageInfo,
620620
w: &mut dyn io::Write,
621621
) -> io::Result<()> {
622-
let coverage::FunctionCoverageInfo { body_span, expressions, mappings, .. } =
623-
function_coverage_info;
622+
let coverage::FunctionCoverageInfo { body_span, mappings, .. } = function_coverage_info;
624623

625624
writeln!(w, "{INDENT}coverage body span: {body_span:?}")?;
626-
for (id, expression) in expressions.iter_enumerated() {
627-
writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?;
628-
}
629625
for coverage::Mapping { kind, span } in mappings {
630626
writeln!(w, "{INDENT}coverage {kind:?} => {span:?};")?;
631627
}

compiler/rustc_middle/src/query/mod.rs

+10-3
Original file line numberDiff line numberDiff line change
@@ -615,9 +615,16 @@ rustc_queries! {
615615
feedable
616616
}
617617

618-
/// Summarizes coverage IDs inserted by the `InstrumentCoverage` MIR pass
619-
/// (for compiler option `-Cinstrument-coverage`), after MIR optimizations
620-
/// have had a chance to potentially remove some of them.
618+
/// Scans through a function's MIR after MIR optimizations, to prepare the
619+
/// information needed by codegen when `-Cinstrument-coverage` is active.
620+
///
621+
/// This includes the details of where to insert `llvm.instrprof.increment`
622+
/// intrinsics, and the expression tables to be embedded in the function's
623+
/// coverage metadata.
624+
///
625+
/// FIXME(Zalathar): This query's purpose has drifted a bit and should
626+
/// probably be renamed, but that can wait until after the potential
627+
/// follow-ups to #136053 have settled down.
621628
///
622629
/// Returns `None` for functions that were not instrumented.
623630
query coverage_ids_info(key: ty::InstanceKind<'tcx>) -> Option<&'tcx mir::coverage::CoverageIdsInfo> {

compiler/rustc_mir_transform/src/coverage/counters.rs

+20-49
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ use std::cmp::Ordering;
22

33
use either::Either;
44
use itertools::Itertools;
5-
use rustc_data_structures::captures::Captures;
65
use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
76
use rustc_data_structures::graph::DirectedGraph;
87
use rustc_index::IndexVec;
@@ -11,31 +10,35 @@ use rustc_middle::mir::coverage::{CounterId, CovTerm, Expression, ExpressionId,
1110

1211
use crate::coverage::counters::balanced_flow::BalancedFlowGraph;
1312
use crate::coverage::counters::node_flow::{
14-
CounterTerm, NodeCounters, make_node_counters, node_flow_data_for_balanced_graph,
13+
CounterTerm, NodeCounters, NodeFlowData, node_flow_data_for_balanced_graph,
1514
};
1615
use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph};
1716

1817
mod balanced_flow;
19-
mod node_flow;
18+
pub(crate) mod node_flow;
2019
mod union_find;
2120

22-
/// Ensures that each BCB node needing a counter has one, by creating physical
23-
/// counters or counter expressions for nodes as required.
24-
pub(super) fn make_bcb_counters(
25-
graph: &CoverageGraph,
26-
bcb_needs_counter: &DenseBitSet<BasicCoverageBlock>,
27-
) -> CoverageCounters {
21+
/// Struct containing the results of [`prepare_bcb_counters_data`].
22+
pub(crate) struct BcbCountersData {
23+
pub(crate) node_flow_data: NodeFlowData<BasicCoverageBlock>,
24+
pub(crate) priority_list: Vec<BasicCoverageBlock>,
25+
}
26+
27+
/// Analyzes the coverage graph to create intermediate data structures that
28+
/// will later be used (during codegen) to create physical counters or counter
29+
/// expressions for each BCB node that needs one.
30+
pub(crate) fn prepare_bcb_counters_data(graph: &CoverageGraph) -> BcbCountersData {
2831
// Create the derived graphs that are necessary for subsequent steps.
2932
let balanced_graph = BalancedFlowGraph::for_graph(graph, |n| !graph[n].is_out_summable);
3033
let node_flow_data = node_flow_data_for_balanced_graph(&balanced_graph);
3134

32-
// Use those graphs to determine which nodes get physical counters, and how
33-
// to compute the execution counts of other nodes from those counters.
35+
// Also create a "priority list" of coverage graph nodes, to help determine
36+
// which ones get physical counters or counter expressions. This needs to
37+
// be done now, because the later parts of the counter-creation process
38+
// won't have access to the original coverage graph.
3439
let priority_list = make_node_flow_priority_list(graph, balanced_graph);
35-
let node_counters = make_node_counters(&node_flow_data, &priority_list);
3640

37-
// Convert the counters into a form suitable for embedding into MIR.
38-
transcribe_counters(&node_counters, bcb_needs_counter)
41+
BcbCountersData { node_flow_data, priority_list }
3942
}
4043

4144
/// Arranges the nodes in `balanced_graph` into a list, such that earlier nodes
@@ -74,7 +77,7 @@ fn make_node_flow_priority_list(
7477
}
7578

7679
// Converts node counters into a form suitable for embedding into MIR.
77-
fn transcribe_counters(
80+
pub(crate) fn transcribe_counters(
7881
old: &NodeCounters<BasicCoverageBlock>,
7982
bcb_needs_counter: &DenseBitSet<BasicCoverageBlock>,
8083
) -> CoverageCounters {
@@ -129,15 +132,15 @@ fn transcribe_counters(
129132
pub(super) struct CoverageCounters {
130133
/// List of places where a counter-increment statement should be injected
131134
/// into MIR, each with its corresponding counter ID.
132-
phys_counter_for_node: FxIndexMap<BasicCoverageBlock, CounterId>,
135+
pub(crate) phys_counter_for_node: FxIndexMap<BasicCoverageBlock, CounterId>,
133136
next_counter_id: CounterId,
134137

135138
/// Coverage counters/expressions that are associated with individual BCBs.
136139
pub(crate) node_counters: IndexVec<BasicCoverageBlock, Option<CovTerm>>,
137140

138141
/// Table of expression data, associating each expression ID with its
139142
/// corresponding operator (+ or -) and its LHS/RHS operands.
140-
expressions: IndexVec<ExpressionId, Expression>,
143+
pub(crate) expressions: IndexVec<ExpressionId, Expression>,
141144
/// Remember expressions that have already been created (or simplified),
142145
/// so that we don't create unnecessary duplicates.
143146
expressions_memo: FxHashMap<Expression, CovTerm>,
@@ -188,12 +191,6 @@ impl CoverageCounters {
188191
self.make_expression(lhs, Op::Subtract, rhs_sum)
189192
}
190193

191-
pub(super) fn num_counters(&self) -> usize {
192-
let num_counters = self.phys_counter_for_node.len();
193-
assert_eq!(num_counters, self.next_counter_id.as_usize());
194-
num_counters
195-
}
196-
197194
fn set_node_counter(&mut self, bcb: BasicCoverageBlock, counter: CovTerm) -> CovTerm {
198195
let existing = self.node_counters[bcb].replace(counter);
199196
assert!(
@@ -202,30 +199,4 @@ impl CoverageCounters {
202199
);
203200
counter
204201
}
205-
206-
/// Returns an iterator over all the nodes in the coverage graph that
207-
/// should have a counter-increment statement injected into MIR, along with
208-
/// each site's corresponding counter ID.
209-
pub(super) fn counter_increment_sites(
210-
&self,
211-
) -> impl Iterator<Item = (CounterId, BasicCoverageBlock)> + Captures<'_> {
212-
self.phys_counter_for_node.iter().map(|(&site, &id)| (id, site))
213-
}
214-
215-
/// Returns an iterator over the subset of BCB nodes that have been associated
216-
/// with a counter *expression*, along with the ID of that expression.
217-
pub(super) fn bcb_nodes_with_coverage_expressions(
218-
&self,
219-
) -> impl Iterator<Item = (BasicCoverageBlock, ExpressionId)> + Captures<'_> {
220-
self.node_counters.iter_enumerated().filter_map(|(bcb, &counter)| match counter {
221-
// Yield the BCB along with its associated expression ID.
222-
Some(CovTerm::Expression(id)) => Some((bcb, id)),
223-
// This BCB is associated with a counter or nothing, so skip it.
224-
Some(CovTerm::Counter { .. } | CovTerm::Zero) | None => None,
225-
})
226-
}
227-
228-
pub(super) fn into_expressions(self) -> IndexVec<ExpressionId, Expression> {
229-
self.expressions
230-
}
231202
}

0 commit comments

Comments
 (0)