Replace ExpressionOperandId with enum Operand

Because the three kinds of operand are now distinguished explicitly, we no
longer need fiddly code to disambiguate counter IDs and expression IDs based on
the total number of counters/expressions in a function.

This does increase the size of operands from 4 bytes to 8 bytes, but that
shouldn't be a big deal since they are mostly stored inside boxed structures,
and the current coverage code is not particularly size-optimized anyway.

Author: Zalathar

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

@@ -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>,
 }
 
@@ -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() = {}
@@ -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,
@@ -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")
@@ -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)
     }
 }

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -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;
@@ -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() {

compiler/rustc_middle/src/mir/coverage.rs

@@ -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]
@@ -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
@@ -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]
@@ -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]
@@ -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 {
+    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(),
+        }
     }
 }
 
@@ -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"),
         }
     }
@@ -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"),
         }

compiler/rustc_middle/src/ty/structural_impls.rs

@@ -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,

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -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,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()
         })
     }
@@ -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);
@@ -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);
                 }
@@ -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)
     }
 
@@ -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!(
@@ -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()`).
@@ -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);
             }
@@ -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,
@@ -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();
@@ -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.