Skip to content

coverage: Replace ExpressionOperandId with enum Operand #113428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 1, 2023
Merged

coverage: Replace ExpressionOperandId with enum Operand #113428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a808d4
Add some line comments to enum `CoverageKind`
Zalathar Jul 4, 2023
1a014d4
Replace `ExpressionOperandId` with enum `Operand`
Zalathar Jun 26, 2023
f103db8
Make coverage expression IDs count up from 0, not down from `u32::MAX`
Zalathar Jun 29, 2023
3920e07
Make coverage counter IDs count up from 0, not 1
Zalathar Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CodeRegion>,
}

Expand Down Expand Up @@ -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<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));
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() = {}
Expand Down Expand Up @@ -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,
Expand All @@ -206,26 +203,23 @@ 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() {
type NewIndexes = IndexSlice<InjectedExpressionIndex, Option<MappedExpressionIndex>>;
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,
// 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));
}
Operand::Expression(id) => {
let index = self.expression_index(id);
self.expressions
.get(index)
.expect("expression id is out of range")
Expand Down Expand Up @@ -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)
}
}
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CodeRegion>,
) -> bool {
if let Some(coverage_context) = self.coverage_context() {
Expand Down
74 changes: 26 additions & 48 deletions compiler/rustc_middle/src/mir/coverage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,6 @@ use rustc_span::Symbol;

use std::fmt::{self, Debug, Formatter};

rustc_index::newtype_index! {
/// An ExpressionOperandId value is assigned directly from either a
/// CounterValueReference.as_u32() (which ascend from 1) or an ExpressionOperandId.as_u32()
/// (which _*descend*_ from u32::MAX). Id value `0` (zero) represents a virtual counter with a
/// constant value of `0`.
#[derive(HashStable)]
#[max = 0xFFFF_FFFF]
#[debug_format = "ExpressionOperandId({})"]
pub struct ExpressionOperandId {
}
}

impl ExpressionOperandId {
/// An expression operand for a "zero counter", as described in the following references:
///
/// * <https://github.com/rust-lang/llvm-project/blob/rustc/13.0-2021-09-30/llvm/docs/CoverageMappingFormat.rst#counter>
/// * <https://github.com/rust-lang/llvm-project/blob/rustc/13.0-2021-09-30/llvm/docs/CoverageMappingFormat.rst#tag>
/// * <https://github.com/rust-lang/llvm-project/blob/rustc/13.0-2021-09-30/llvm/docs/CoverageMappingFormat.rst#counter-expressions>
///
/// This operand can be used to count two or more separate code regions with a single counter,
/// if they run sequentially with no branches, by injecting the `Counter` in a `BasicBlock` for
/// one of the code regions, and inserting `CounterExpression`s ("add ZERO to the counter") in
/// the coverage map for the other code regions.
pub const ZERO: Self = Self::from_u32(0);
}

rustc_index::newtype_index! {
#[derive(HashStable)]
#[max = 0xFFFF_FFFF]
Expand All @@ -39,7 +13,7 @@ rustc_index::newtype_index! {
}

impl CounterValueReference {
/// Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO.
/// Counters start at 1 for historical reasons.
pub const START: Self = Self::from_u32(1);

/// Returns explicitly-requested zero-based version of the counter id, used
Expand All @@ -52,8 +26,6 @@ impl CounterValueReference {
}

rustc_index::newtype_index! {
/// InjectedExpressionId.as_u32() converts to ExpressionOperandId.as_u32()
///
/// Values descend from u32::MAX.
#[derive(HashStable)]
#[max = 0xFFFF_FFFF]
Expand All @@ -62,8 +34,6 @@ rustc_index::newtype_index! {
}

rustc_index::newtype_index! {
/// InjectedExpressionIndex.as_u32() translates to u32::MAX - ExpressionOperandId.as_u32()
///
/// Values ascend from 0.
#[derive(HashStable)]
#[max = 0xFFFF_FFFF]
Expand All @@ -81,17 +51,25 @@ rustc_index::newtype_index! {
pub struct MappedExpressionIndex {}
}

impl From<CounterValueReference> for ExpressionOperandId {
#[inline]
fn from(v: CounterValueReference) -> ExpressionOperandId {
ExpressionOperandId::from(v.as_u32())
}
/// Operand of a coverage-counter expression.
///
/// Operands can be a constant zero value, an actual coverage counter, or another
/// expression. Counter/expression operands are referred to by ID.
#[derive(Copy, Clone, PartialEq, Eq)]
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub enum Operand {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe we could rename this to CoverageOperand (or something like that), just because mir::Operand is quite common and it could be easy to get mixed up when reading the coverage code for the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I haven't interacted directly with actual MIR very much, so this didn't occur to me.

I have a bunch of other changes stacked up behind this one that use Operand, so to avoid churning those I definitely won't be renaming it in the near future. But it's something to bear in mind once my current coverage work has settled down.

Zero,
Counter(CounterValueReference),
Expression(InjectedExpressionId),
}

impl From<InjectedExpressionId> for ExpressionOperandId {
#[inline]
fn from(v: InjectedExpressionId) -> ExpressionOperandId {
ExpressionOperandId::from(v.as_u32())
impl Debug for Operand {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
Self::Zero => write!(f, "Zero"),
Self::Counter(id) => f.debug_tuple("Counter").field(&id.as_u32()).finish(),
Self::Expression(id) => f.debug_tuple("Expression").field(&id.as_u32()).finish(),
}
}
}

Expand All @@ -107,19 +85,19 @@ pub enum CoverageKind {
/// 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,
lhs: ExpressionOperandId,
lhs: Operand,
op: Op,
rhs: ExpressionOperandId,
rhs: Operand,
},
Unreachable,
}

impl CoverageKind {
pub fn as_operand_id(&self) -> ExpressionOperandId {
pub fn as_operand(&self) -> Operand {
use CoverageKind::*;
match *self {
Counter { id, .. } => ExpressionOperandId::from(id),
Expression { id, .. } => ExpressionOperandId::from(id),
Counter { id, .. } => Operand::Counter(id),
Expression { id, .. } => Operand::Expression(id),
Unreachable => bug!("Unreachable coverage cannot be part of an expression"),
}
}
Expand All @@ -136,14 +114,14 @@ impl Debug for CoverageKind {
Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()),
Expression { id, lhs, op, rhs } => write!(
fmt,
"Expression({:?}) = {} {} {}",
"Expression({:?}) = {:?} {} {:?}",
id.index(),
lhs.index(),
lhs,
match op {
Op::Add => "+",
Op::Subtract => "-",
},
rhs.index(),
rhs,
),
Unreachable => write!(fmt, "Unreachable"),
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/ty/structural_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ TrivialTypeTraversalAndLiftImpls! {
::rustc_hir::Unsafety,
::rustc_target::asm::InlineAsmRegOrRegClass,
::rustc_target::spec::abi::Abi,
crate::mir::coverage::ExpressionOperandId,
crate::mir::coverage::CounterValueReference,
crate::mir::coverage::InjectedExpressionId,
crate::mir::coverage::InjectedExpressionIndex,
Expand Down
26 changes: 13 additions & 13 deletions compiler/rustc_mir_transform/src/coverage/counters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ impl CoverageCounters {

fn make_expression<F>(
&mut self,
lhs: ExpressionOperandId,
lhs: Operand,
op: Op,
rhs: ExpressionOperandId,
rhs: Operand,
debug_block_label_fn: F,
) -> CoverageKind
where
Expand All @@ -81,13 +81,13 @@ impl CoverageCounters {
expression
}

pub fn make_identity_counter(&mut self, counter_operand: ExpressionOperandId) -> CoverageKind {
pub fn make_identity_counter(&mut self, counter_operand: Operand) -> CoverageKind {
let some_debug_block_label = if self.debug_counters.is_enabled() {
self.debug_counters.some_block_label(counter_operand).cloned()
} else {
None
};
self.make_expression(counter_operand, Op::Add, ExpressionOperandId::ZERO, || {
self.make_expression(counter_operand, Op::Add, Operand::Zero, || {
some_debug_block_label.clone()
})
}
Expand Down Expand Up @@ -199,7 +199,7 @@ impl<'a> BcbCounters<'a> {
&mut self,
traversal: &mut TraverseCoverageGraphWithLoops,
branching_bcb: BasicCoverageBlock,
branching_counter_operand: ExpressionOperandId,
branching_counter_operand: Operand,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<(), Error> {
let branches = self.bcb_branches(branching_bcb);
Expand Down Expand Up @@ -261,7 +261,7 @@ impl<'a> BcbCounters<'a> {
" [new intermediate expression: {}]",
self.format_counter(&intermediate_expression)
);
let intermediate_expression_operand = intermediate_expression.as_operand_id();
let intermediate_expression_operand = intermediate_expression.as_operand();
collect_intermediate_expressions.push(intermediate_expression);
some_sumup_counter_operand.replace(intermediate_expression_operand);
}
Expand Down Expand Up @@ -298,7 +298,7 @@ impl<'a> BcbCounters<'a> {
&mut self,
bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<ExpressionOperandId, Error> {
) -> Result<Operand, Error> {
self.recursive_get_or_make_counter_operand(bcb, collect_intermediate_expressions, 1)
}

Expand All @@ -307,7 +307,7 @@ impl<'a> BcbCounters<'a> {
bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
debug_indent_level: usize,
) -> Result<ExpressionOperandId, Error> {
) -> Result<Operand, Error> {
// If the BCB already has a counter, return it.
if let Some(counter_kind) = self.basic_coverage_blocks[bcb].counter() {
debug!(
Expand All @@ -316,7 +316,7 @@ impl<'a> BcbCounters<'a> {
bcb,
self.format_counter(counter_kind),
);
return Ok(counter_kind.as_operand_id());
return Ok(counter_kind.as_operand());
}

// A BCB with only one incoming edge gets a simple `Counter` (via `make_counter()`).
Expand Down Expand Up @@ -383,7 +383,7 @@ impl<'a> BcbCounters<'a> {
NESTED_INDENT.repeat(debug_indent_level),
self.format_counter(&intermediate_expression)
);
let intermediate_expression_operand = intermediate_expression.as_operand_id();
let intermediate_expression_operand = intermediate_expression.as_operand();
collect_intermediate_expressions.push(intermediate_expression);
some_sumup_edge_counter_operand.replace(intermediate_expression_operand);
}
Expand All @@ -408,7 +408,7 @@ impl<'a> BcbCounters<'a> {
from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
) -> Result<ExpressionOperandId, Error> {
) -> Result<Operand, Error> {
self.recursive_get_or_make_edge_counter_operand(
from_bcb,
to_bcb,
Expand All @@ -423,7 +423,7 @@ impl<'a> BcbCounters<'a> {
to_bcb: BasicCoverageBlock,
collect_intermediate_expressions: &mut Vec<CoverageKind>,
debug_indent_level: usize,
) -> Result<ExpressionOperandId, Error> {
) -> Result<Operand, Error> {
// If the source BCB has only one successor (assumed to be the given target), an edge
// counter is unnecessary. Just get or make a counter for the source BCB.
let successors = self.bcb_successors(from_bcb).iter();
Expand All @@ -444,7 +444,7 @@ impl<'a> BcbCounters<'a> {
to_bcb,
self.format_counter(counter_kind)
);
return Ok(counter_kind.as_operand_id());
return Ok(counter_kind.as_operand());
}

// Make a new counter to count this edge.
Expand Down
Loading