Rollup merge of #113428 - Zalathar:operand, r=davidtwco

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).

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
+    /// Constructs a new `Counter` of kind `CounterValueReference`.
-    /// the given 1-based counter_id to the required 0-based equivalent for
+    pub fn counter_value_reference(counter_id: CounterId) -> Self {
-    /// the `Counter` encoding.
+        Self { kind: CounterKind::CounterValueReference, id: counter_id.as_u32() }
-    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 `Expression`.

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

@@ -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,
+    CodeRegion, CounterId, ExpressionId, MappedExpressionIndex, Op, Operand,
-    InjectedExpressionIndex, MappedExpressionIndex, Op,
 };
 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
+/// for a given Function. This struct also stores the `function_source_hash`,
-/// can both be operands in an expression. 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
@@ -34,8 +32,8 @@ pub struct FunctionCoverage<'tcx> {
     instance: Instance<'tcx>,
     source_hash: u64,
     is_used: bool,
-    counters: IndexVec<CounterValueReference, Option<CodeRegion>>,
+    counters: IndexVec<CounterId, Option<CodeRegion>>,
-    expressions: IndexVec<InjectedExpressionIndex, Option<Expression>>,
+    expressions: IndexVec<ExpressionId, Option<Expression>>,
     unreachable_regions: Vec<CodeRegion>,
 }
 
@@ -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
+    /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity
-    /// IDs will not overlap with the range of counter IDs. Counters and expressions can be added in
+    /// between operands that are counter IDs and operands that are expression IDs.
-    /// 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.
     pub fn add_counter_expression(
         &mut self,
-        expression_id: InjectedExpressionId,
+        expression_id: ExpressionId,
-        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));
         debug_assert!(
-            expression_index.as_usize() < self.expressions.len(),
+            expression_id.as_usize() < self.expressions.len(),
-            "expression_index {} is out of range for 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,
@@ -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
+        // and value.
-        // 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`.
         //
         // 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).
@@ -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<
+        type NewIndexes = IndexSlice<ExpressionId, Option<MappedExpressionIndex>>;
-            InjectedExpressionIndex,
+        let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand {
-            Option<MappedExpressionIndex>,
+            Operand::Zero => Some(Counter::zero()),
-        >,
+            Operand::Counter(id) => Some(Counter::counter_value_reference(id)),
-                             id: ExpressionOperandId| {
+            Operand::Expression(id) => {
-            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));
                 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))
             }
         };
 
@@ -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)
-    }
 }

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::{
+use rustc_middle::mir::coverage::{CodeRegion, CounterId, CoverageKind, ExpressionId, Op, Operand};
-    CodeRegion, CounterValueReference, CoverageKind, ExpressionOperandId, InjectedExpressionId, Op,
-};
 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() {
@@ -202,10 +200,10 @@ impl<'tcx> Builder<'_, '_, 'tcx> {
     fn add_coverage_counter_expression(
         &mut self,
         instance: Instance<'tcx>,
-        id: InjectedExpressionId,
+        id: ExpressionId,
-        lhs: ExpressionOperandId,
+        lhs: Operand,
         op: Op,
-        rhs: ExpressionOperandId,
+        rhs: Operand,
         region: Option<CodeRegion>,
     ) -> bool {
         if let Some(coverage_context) = self.coverage_context() {

compiler/rustc_middle/src/mir/coverage.rs

@@ -6,69 +6,43 @@ use rustc_span::Symbol;
 use std::fmt::{self, Debug, Formatter};
 
 rustc_index::newtype_index! {
-    /// An ExpressionOperandId value is assigned directly from either a
+    /// ID of a coverage counter. Values ascend from 0.
-    /// 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
+    /// Note that LLVM handles counter IDs as `uint32_t`, so there is no need
-    /// constant value of `0`.
+    /// to use a larger representation on the Rust side.
     #[derive(HashStable)]
     #[max = 0xFFFF_FFFF]
-    #[debug_format = "ExpressionOperandId({})"]
+    #[debug_format = "CounterId({})"]
-    pub struct ExpressionOperandId {
+    pub struct CounterId {}
-    }
 }
 
-impl ExpressionOperandId {
+impl CounterId {
-    /// An expression operand for a "zero counter", as described in the following references:
+    pub const START: Self = Self::from_u32(0);
-    ///
-    /// * <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! {
+    #[inline(always)]
-    #[derive(HashStable)]
+    pub fn next_id(self) -> Self {
-    #[max = 0xFFFF_FFFF]
+        Self::from_u32(self.as_u32() + 1)
-    #[debug_format = "CounterValueReference({})"]
-    pub struct CounterValueReference {}
-}
-
-impl CounterValueReference {
-    /// Counters start at 1 to reserve 0 for ExpressionOperandId::ZERO.
-    pub const START: Self = Self::from_u32(1);
-
-    /// 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
     }
 }
 
 rustc_index::newtype_index! {
-    /// InjectedExpressionId.as_u32() converts to ExpressionOperandId.as_u32()
+    /// ID of a coverage-counter expression. Values ascend from 0.
     ///
-    /// Values descend from u32::MAX.
+    /// 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({})"]
+    #[debug_format = "ExpressionId({})"]
-    pub struct InjectedExpressionId {}
+    pub struct ExpressionId {}
 }
 
-rustc_index::newtype_index! {
+impl ExpressionId {
-    /// InjectedExpressionIndex.as_u32() translates to u32::MAX - ExpressionOperandId.as_u32()
+    pub const START: Self = Self::from_u32(0);
-    ///
+
-    /// Values ascend from 0.
+    #[inline(always)]
-    #[derive(HashStable)]
+    pub fn next_id(self) -> Self {
-    #[max = 0xFFFF_FFFF]
+        Self::from_u32(self.as_u32() + 1)
-    #[debug_format = "InjectedExpressionIndex({})"]
+    }
-    pub struct InjectedExpressionIndex {}
 }
 
 rustc_index::newtype_index! {
@@ -81,17 +55,25 @@ rustc_index::newtype_index! {
     pub struct MappedExpressionIndex {}
 }
 
-impl From<CounterValueReference> for ExpressionOperandId {
+/// Operand of a coverage-counter expression.
-    #[inline]
+///
-    fn from(v: CounterValueReference) -> ExpressionOperandId {
+/// Operands can be a constant zero value, an actual coverage counter, or another
-        ExpressionOperandId::from(v.as_u32())
+/// expression. Counter/expression operands are referred to by ID.
-    }
+#[derive(Copy, Clone, PartialEq, Eq)]
+#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
+pub enum Operand {
+    Zero,
+    Counter(CounterId),
+    Expression(ExpressionId),
 }
 
-impl From<InjectedExpressionId> for ExpressionOperandId {
+impl Debug for Operand {
-    #[inline]
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-    fn from(v: InjectedExpressionId) -> ExpressionOperandId {
+        match self {
-        ExpressionOperandId::from(v.as_u32())
+            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(),
+        }
     }
 }
 
@@ -99,23 +81,27 @@ impl From<InjectedExpressionId> for ExpressionOperandId {
 pub enum CoverageKind {
     Counter {
         function_source_hash: u64,
-        id: CounterValueReference,
+        /// ID of this counter within its enclosing function.
+        /// Expressions in the same function can refer to it as an operand.
+        id: CounterId,
     },
     Expression {
-        id: InjectedExpressionId,
+        /// ID of this coverage-counter expression within its enclosing function.
-        lhs: ExpressionOperandId,
+        /// Other expressions in the same function can refer to it as an operand.
+        id: ExpressionId,
+        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),
+            Counter { id, .. } => Operand::Counter(id),
-            Expression { id, .. } => ExpressionOperandId::from(id),
+            Expression { id, .. } => Operand::Expression(id),
             Unreachable => bug!("Unreachable coverage cannot be part of an expression"),
         }
     }
@@ -132,14 +118,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"),
         }

compiler/rustc_middle/src/ty/structural_impls.rs

@@ -470,10 +470,8 @@ TrivialTypeTraversalAndLiftImpls! {
     ::rustc_hir::Unsafety,
     ::rustc_target::asm::InlineAsmRegOrRegClass,
     ::rustc_target::spec::abi::Abi,
-    crate::mir::coverage::ExpressionOperandId,
+    crate::mir::coverage::CounterId,
-    crate::mir::coverage::CounterValueReference,
+    crate::mir::coverage::ExpressionId,
-    crate::mir::coverage::InjectedExpressionId,
-    crate::mir::coverage::InjectedExpressionIndex,
     crate::mir::coverage::MappedExpressionIndex,
     crate::mir::Local,
     crate::mir::Promoted,

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -16,8 +16,8 @@ use rustc_middle::mir::coverage::*;
 /// `Coverage` statements.
 pub(super) struct CoverageCounters {
     function_source_hash: u64,
-    next_counter_id: u32,
+    next_counter_id: CounterId,
-    num_expressions: u32,
+    next_expression_id: ExpressionId,
     pub debug_counters: DebugCounters,
 }
 
@@ -25,8 +25,8 @@ 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,
-            num_expressions: 0,
+            next_expression_id: ExpressionId::START,
             debug_counters: DebugCounters::new(),
         }
     }
@@ -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
@@ -81,33 +81,30 @@ 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()
         })
     }
 
     /// Counter IDs start from one and go up.
-    fn next_counter(&mut self) -> CounterValueReference {
+    fn next_counter(&mut self) -> CounterId {
-        assert!(self.next_counter_id < u32::MAX - self.num_expressions);
         let next = self.next_counter_id;
-        self.next_counter_id += 1;
+        self.next_counter_id = next.next_id();
-        CounterValueReference::from(next)
+        next
     }
 
-    /// Expression IDs start from u32::MAX and go down because an Expression can reference
+    /// Expression IDs start from 0 and go up.
-    /// (add or subtract counts) of both Counter regions and Expression regions. The counter
+    /// (Counter IDs and Expression IDs are distinguished by the `Operand` enum.)
-    /// expression operand IDs must be unique across both types.
+    fn next_expression(&mut self) -> ExpressionId {
-    fn next_expression(&mut self) -> InjectedExpressionId {
+        let next = self.next_expression_id;
-        assert!(self.next_counter_id < u32::MAX - self.num_expressions);
+        self.next_expression_id = next.next_id();
-        let next = u32::MAX - self.num_expressions;
+        next
-        self.num_expressions += 1;
-        InjectedExpressionId::from(next)
     }
 }
 
@@ -199,7 +196,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);
@@ -261,7 +258,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);
                 }
@@ -298,7 +295,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)
     }
 
@@ -307,7 +304,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!(
@@ -316,7 +313,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()`).
@@ -383,7 +380,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);
             }
@@ -408,7 +405,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,
@@ -423,7 +420,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();
@@ -444,7 +441,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.

compiler/rustc_mir_transform/src/coverage/debug.rs

@@ -246,7 +246,7 @@ impl Default for ExpressionFormat {
     }
 }
 
-/// If enabled, this struct maintains a map from `CoverageKind` IDs (as `ExpressionOperandId`) to
+/// If enabled, this struct maintains a map from `CoverageKind` IDs (as `Operand`) to
 /// the `CoverageKind` data and optional label (normally, the counter's associated
 /// `BasicCoverageBlock` format string, if any).
 ///
@@ -258,7 +258,7 @@ impl Default for ExpressionFormat {
 /// `DebugCounters` supports a recursive rendering of `Expression` counters, so they can be
 /// presented as nested expressions such as `(bcb3 - (bcb0 + bcb1))`.
 pub(super) struct DebugCounters {
-    some_counters: Option<FxHashMap<ExpressionOperandId, DebugCounter>>,
+    some_counters: Option<FxHashMap<Operand, DebugCounter>>,
 }
 
 impl DebugCounters {
@@ -277,14 +277,14 @@ impl DebugCounters {
 
     pub fn add_counter(&mut self, counter_kind: &CoverageKind, some_block_label: Option<String>) {
         if let Some(counters) = &mut self.some_counters {
-            let id = counter_kind.as_operand_id();
+            let id = counter_kind.as_operand();
             counters
                 .try_insert(id, DebugCounter::new(counter_kind.clone(), some_block_label))
                 .expect("attempt to add the same counter_kind to DebugCounters more than once");
         }
     }
 
-    pub fn some_block_label(&self, operand: ExpressionOperandId) -> Option<&String> {
+    pub fn some_block_label(&self, operand: Operand) -> Option<&String> {
         self.some_counters.as_ref().and_then(|counters| {
             counters.get(&operand).and_then(|debug_counter| debug_counter.some_block_label.as_ref())
         })
@@ -323,24 +323,24 @@ impl DebugCounters {
             }
         }
 
-        let id = counter_kind.as_operand_id();
+        let id = counter_kind.as_operand();
         if self.some_counters.is_some() && (counter_format.block || !counter_format.id) {
             let counters = self.some_counters.as_ref().unwrap();
             if let Some(DebugCounter { some_block_label: Some(block_label), .. }) =
                 counters.get(&id)
             {
                 return if counter_format.id {
-                    format!("{}#{}", block_label, id.index())
+                    format!("{}#{:?}", block_label, id)
                 } else {
                     block_label.to_string()
                 };
             }
         }
-        format!("#{}", id.index())
+        format!("#{:?}", id)
     }
 
-    fn format_operand(&self, operand: ExpressionOperandId) -> String {
+    fn format_operand(&self, operand: Operand) -> String {
-        if operand.index() == 0 {
+        if matches!(operand, Operand::Zero) {
             return String::from("0");
         }
         if let Some(counters) = &self.some_counters {
@@ -358,7 +358,7 @@ impl DebugCounters {
                 return self.format_counter_kind(counter_kind);
             }
         }
-        format!("#{}", operand.index())
+        format!("#{:?}", operand)
     }
 }
 
@@ -485,8 +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:
+    some_used_expression_operands: Option<FxHashMap<Operand, Vec<ExpressionId>>>,
-        Option<FxHashMap<ExpressionOperandId, Vec<InjectedExpressionId>>>,
     some_unused_expressions:
         Option<Vec<(CoverageKind, Option<BasicCoverageBlock>, BasicCoverageBlock)>>,
 }
@@ -517,7 +516,7 @@ impl UsedExpressions {
 
     pub fn expression_is_used(&self, expression: &CoverageKind) -> bool {
         if let Some(used_expression_operands) = self.some_used_expression_operands.as_ref() {
-            used_expression_operands.contains_key(&expression.as_operand_id())
+            used_expression_operands.contains_key(&expression.as_operand())
         } else {
             false
         }
@@ -530,7 +529,7 @@ impl UsedExpressions {
         target_bcb: BasicCoverageBlock,
     ) {
         if let Some(used_expression_operands) = self.some_used_expression_operands.as_ref() {
-            if !used_expression_operands.contains_key(&expression.as_operand_id()) {
+            if !used_expression_operands.contains_key(&expression.as_operand()) {
                 self.some_unused_expressions.as_mut().unwrap().push((
                     expression.clone(),
                     edge_from_bcb,

compiler/rustc_mir_transform/src/coverage/graph.rs

@@ -345,10 +345,7 @@ impl BasicCoverageBlockData {
         &mir_body[self.last_bb()].terminator()
     }
 
-    pub fn set_counter(
+    pub fn set_counter(&mut self, counter_kind: CoverageKind) -> Result<Operand, Error> {
-        &mut self,
-        counter_kind: CoverageKind,
-    ) -> Result<ExpressionOperandId, Error> {
         debug_assert!(
             // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also
             // have an expression (to be injected into an existing `BasicBlock` represented by this
@@ -356,7 +353,7 @@ impl BasicCoverageBlockData {
             self.edge_from_bcbs.is_none() || counter_kind.is_expression(),
             "attempt to add a `Counter` to a BCB target with existing incoming edge counters"
         );
-        let operand = counter_kind.as_operand_id();
+        let operand = counter_kind.as_operand();
         if let Some(replaced) = self.counter_kind.replace(counter_kind) {
             Error::from_string(format!(
                 "attempt to set a BasicCoverageBlock coverage counter more than once; \
@@ -381,7 +378,7 @@ impl BasicCoverageBlockData {
         &mut self,
         from_bcb: BasicCoverageBlock,
         counter_kind: CoverageKind,
-    ) -> Result<ExpressionOperandId, Error> {
+    ) -> Result<Operand, Error> {
         if level_enabled!(tracing::Level::DEBUG) {
             // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also
             // have an expression (to be injected into an existing `BasicBlock` represented by this
@@ -393,7 +390,7 @@ impl BasicCoverageBlockData {
                 ));
             }
         }
-        let operand = counter_kind.as_operand_id();
+        let operand = counter_kind.as_operand();
         if let Some(replaced) =
             self.edge_from_bcbs.get_or_insert_default().insert(from_bcb, counter_kind)
         {

compiler/rustc_mir_transform/src/coverage/mod.rs

@@ -304,7 +304,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
             let counter_kind = if let Some(&counter_operand) = bcb_counters[bcb].as_ref() {
                 self.coverage_counters.make_identity_counter(counter_operand)
             } else if let Some(counter_kind) = self.bcb_data_mut(bcb).take_counter() {
-                bcb_counters[bcb] = Some(counter_kind.as_operand_id());
+                bcb_counters[bcb] = Some(counter_kind.as_operand());
                 debug_used_expressions.add_expression_operands(&counter_kind);
                 counter_kind
             } else {

compiler/rustc_mir_transform/src/coverage/query.rs

@@ -43,43 +43,25 @@ struct CoverageVisitor {
 }
 
 impl CoverageVisitor {
-    /// Updates `num_counters` to the maximum encountered zero-based counter_id plus 1. Note the
+    /// Updates `num_counters` to the maximum encountered counter ID plus 1.
-    /// final computed number of counters should be the number of all `CoverageKind::Counter`
-    /// statements in the MIR *plus one* for the implicit `ZERO` counter.
     #[inline(always)]
-    fn update_num_counters(&mut self, counter_id: u32) {
+    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);
     }
 
-    /// Computes an expression index for each expression ID, and updates `num_expressions` to the
+    /// Updates `num_expressions` to the maximum encountered expression ID plus 1.
-    /// maximum encountered index plus 1.
     #[inline(always)]
-    fn update_num_expressions(&mut self, expression_id: u32) {
+    fn update_num_expressions(&mut self, expression_id: ExpressionId) {
-        let expression_index = u32::MAX - expression_id;
+        let expression_id = expression_id.as_u32();
-        self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1);
+        self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_id + 1);
     }
 
-    fn update_from_expression_operand(&mut self, operand_id: u32) {
+    fn update_from_expression_operand(&mut self, operand: Operand) {
-        if operand_id >= self.info.num_counters {
+        match operand {
-            let operand_as_expression_index = u32::MAX - operand_id;
+            Operand::Counter(id) => self.update_num_counters(id),
-            if operand_as_expression_index >= self.info.num_expressions {
+            Operand::Expression(id) => self.update_num_expressions(id),
-                // The operand ID is outside the known range of counter IDs and also outside the
+            Operand::Zero => {}
-                // known range of expression IDs. In either case, the result of a missing operand
-                // (if and when used in an expression) will be zero, so from a computation
-                // perspective, it doesn't matter whether it is interpreted as a counter or an
-                // expression.
-                //
-                // However, the `num_counters` and `num_expressions` query results are used to
-                // allocate arrays when generating the coverage map (during codegen), so choose
-                // the type that grows either `num_counters` or `num_expressions` the least.
-                if operand_id - self.info.num_counters
-                    < operand_as_expression_index - self.info.num_expressions
-                {
-                    self.update_num_counters(operand_id)
-                } else {
-                    self.update_num_expressions(operand_id)
-                }
-            }
         }
     }
 
@@ -100,19 +82,15 @@ impl CoverageVisitor {
         if self.add_missing_operands {
             match coverage.kind {
                 CoverageKind::Expression { lhs, rhs, .. } => {
-                    self.update_from_expression_operand(u32::from(lhs));
+                    self.update_from_expression_operand(lhs);
-                    self.update_from_expression_operand(u32::from(rhs));
+                    self.update_from_expression_operand(rhs);
                 }
                 _ => {}
             }
         } else {
             match coverage.kind {
-                CoverageKind::Counter { id, .. } => {
+                CoverageKind::Counter { id, .. } => self.update_num_counters(id),
-                    self.update_num_counters(u32::from(id));
+                CoverageKind::Expression { id, .. } => self.update_num_expressions(id),
-                }
-                CoverageKind::Expression { id, .. } => {
-                    self.update_num_expressions(u32::from(id));
-                }
                 _ => {}
             }
         }
@@ -123,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: 0, num_expressions: 0 },
-        info: CoverageInfo { num_counters: 1, num_expressions: 0 },
         add_missing_operands: false,
     };
 

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"),

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=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br align="left"/>    19:5-19:9: @0[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:18:1 - 20:2<br align="left"/>    20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
+    bcb0__Cov_0_4 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 18:1-20:2<br align="left"/>    19:5-19:9: @0[0]: Coverage::Counter(0) for $DIR/coverage_graphviz.rs:18:1 - 20:2<br align="left"/>    20:2-20:2: @0.Return: return</td></tr><tr><td align="left" balign="left">bb0: Return</td></tr></table>>];
 }

tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot

@@ -2,8 +2,8 @@ digraph Cov_0_3 {
     graph [fontname="Courier, monospace"];
     node [fontname="Courier, monospace"];
     edge [fontname="Courier, monospace"];
-    bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/>    13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>];
+    bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/>    13:10-13:10: @5[0]: Coverage::Counter(1) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>];
-    bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18<br align="left"/>    12:13-12:18: @4[0]: Coverage::Expression(4294967293) = 4294967294 + 0 for $DIR/coverage_graphviz.rs:15:1 - 15:2<br align="left"/>Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2<br align="left"/>    15:2-15:2: @4.Return: return</td></tr><tr><td align="left" balign="left">bb4: Return</td></tr></table>>];
+    bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18<br align="left"/>    12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2<br align="left"/>Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2<br align="left"/>    15:2-15:2: @4.Return: return</td></tr><tr><td align="left" balign="left">bb4: Return</td></tr></table>>];
     bcb1__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb1</td></tr><tr><td align="left" balign="left">Expression(bcb0 + bcb3) at 10:5-11:17<br align="left"/>    11:12-11:17: @2.Call: _2 = bar() -&gt; [return: bb3, unwind: bb6]</td></tr><tr><td align="left" balign="left">bb1: FalseUnwind<br align="left"/>bb2: Call</td></tr><tr><td align="left" balign="left">bb3: SwitchInt</td></tr></table>>];
     bcb0__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 9:1-9:11<br align="left"/>    </td></tr><tr><td align="left" balign="left">bb0: Goto</td></tr></table>>];
     bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>];

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;
       }

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(4294967295) = 1 + 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];
       }
   
@@ -27,15 +27,15 @@
       }
   
       bb4: {
-+         Coverage::Expression(4294967293) = 4294967294 + 0 for /the/src/instrument_coverage.rs:17:1 - 17:2;
++         Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2;
-+         Coverage::Expression(4294967294) = 4294967295 - 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;