Skip to content

Commit

Permalink
Rollup merge of #113428 - Zalathar:operand, r=davidtwco
Browse files Browse the repository at this point in the history 
coverage: Replace `ExpressionOperandId` with enum `Operand`

*This is one step in my larger coverage refactoring ambitions described at <https://github.com/rust-lang/compiler-team/issues/645>.*

LLVM coverage has a concept of “mapping expressions” that allow a span's execution count to be computed as a simple arithmetic expression over other counters/expressions, instead of requiring a dedicated physical counter for every control-flow branch.

These expressions have an operator (`+` or `-`) and two operands. Operands are currently represented as `ExpressionOperandId`, which wraps a `u32` with the following semantics:

- 0 represents a special counter that always has a value of zero
- Values ascending from 1 represent counter IDs
- Values descending from `u32::MAX` represent the IDs of other expressions

---

This change replaces that whole `ExpressionOperandId` scheme with a simple enum that explicitly distinguishes between the three cases.

This lets us remove a lot of fiddly code for dealing with the different operand kinds:
- Previously it was only possible to distinguish between counter-ID operands and expression-ID operands by comparing the operand ID with the total number of counters in a function. This is unnecessary now that the enum distinguishes them explicitly.
- There's no need for expression IDs to descend from `u32::MAX` and then get translated into zero-based indices in certain places. Now that they ascend from zero, they can be used as indices directly.
- There's no need to reserve ID number 0 for the special zero operand, since it can just have its own variant in the enum, so counter IDs can count up from 0.

(Making counter IDs ascend from 0 also lets us fix an off-by-one error in the query for counting the total number of counters, which would cause LLVM to emit an extra unused counter for every instrumented function.)

---

This PR may be easiest to review as individual patches, since that breaks it up into clearly distinct parts:
- Replace a `u32` wrapper with an explicit enum, without changing the semantics of the underlying IDs being stored.
- Change the numbering scheme used by `Operand::Expression` to make expression IDs ascend from 0 (instead of descending from `u32::MAX`).
- Change the numbering scheme used by `Operand::Counter` to make counter IDs ascend from 0 (instead of ascending from 1).
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Aug 1, 2023
2 parents c97af34 + 3920e07 commit 52bfceb
Show file tree
Hide file tree
Showing 15 changed files with 163 additions and 250 deletions.
10 changes: 4 additions & 6 deletions compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs
View file
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -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`.
Expand Down
87 changes: 25 additions & 62 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,22 @@ 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, CounterId, ExpressionId, 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<CodeRegion>,
}

/// 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
Expand All @@ -34,8 +32,8 @@ pub struct FunctionCoverage<'tcx> {
instance: Instance<'tcx>,
source_hash: u64,
is_used: bool,
counters: IndexVec<CounterValueReference, Option<CodeRegion>>,
expressions: IndexVec<InjectedExpressionIndex, Option<Expression>>,
counters: IndexVec<CounterId, Option<CodeRegion>>,
expressions: IndexVec<ExpressionId, Option<Expression>>,
unreachable_regions: Vec<CodeRegion>,
}

Expand Down Expand Up @@ -82,48 +80,36 @@ 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");
}
}

/// 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,
lhs: ExpressionOperandId,
expression_id: ExpressionId,
lhs: Operand,
op: Op,
rhs: ExpressionOperandId,
rhs: Operand,
region: Option<CodeRegion>,
) {
debug!(
"add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}",
expression_id, lhs, op, rhs, region
);
let expression_index = self.expression_index(u32::from(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,
Expand Down Expand Up @@ -186,14 +172,11 @@ 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,
// 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).
Expand All @@ -206,34 +189,19 @@ 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<MappedExpressionIndex>,
>,
id: ExpressionOperandId| {
if id == ExpressionOperandId::ZERO {
Some(Counter::zero())
} else if id.index() < self.counters.len() {
debug_assert!(
id.index() > 0,
"ExpressionOperandId 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))
} else {
let index = self.expression_index(u32::from(id));
type NewIndexes = IndexSlice<ExpressionId, Option<MappedExpressionIndex>>;
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(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))
}
};

Expand Down Expand Up @@ -340,9 +308,4 @@ impl<'tcx> FunctionCoverage<'tcx> {
fn unreachable_regions(&self) -> impl Iterator<Item = (Counter, &CodeRegion)> {
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);
InjectedExpressionIndex::from(u32::MAX - id_descending_from_max)
}
}
16 changes: 7 additions & 9 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, ExpressionOperandId, InjectedExpressionId, Op,
};
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};
Expand All @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
Expand All @@ -202,10 +200,10 @@ impl<'tcx> Builder<'_, '_, 'tcx> {
fn add_coverage_counter_expression(
&mut self,
instance: Instance<'tcx>,
id: InjectedExpressionId,
lhs: ExpressionOperandId,
id: ExpressionId,
lhs: Operand,
op: Op,
rhs: ExpressionOperandId,
rhs: Operand,
region: Option<CodeRegion>,
) -> bool {
if let Some(coverage_context) = self.coverage_context() {
Expand Down
Loading

0 comments on commit 52bfceb

Please sign in to comment.