[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

NAKAMURA Takumi via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 13 05:38:37 PST 2024


https://github.com/chapuni updated https://github.com/llvm/llvm-project/pull/81227

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH 1/3] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp      | 69 +++++++++--------
 .../ProfileData/Coverage/CoverageMapping.h    | 77 ++++++++++++-------
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++++++------
 .../Coverage/CoverageMappingReader.cpp        | 17 ++--
 .../Coverage/CoverageMappingWriter.cpp        | 23 ++++--
 .../ProfileData/CoverageMappingTest.cpp       |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) {
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+    const auto *DecisionParams =
+        std::get_if<MCDCDecisionParameters>(&MCDCParams);
+    assert(!DecisionParams || DecisionParams->NumConditions > 0);
+    return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+    return CounterMappingRegion::getParams<const MCDCDecisionParameters>(
+        MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
             SR.ColumnEnd));
       } else if (Region.isBranch()) {
         MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-            Region.getCounter(), Region.getFalseCounter(),
-            Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-            SR.LineEnd, SR.ColumnEnd));
+            Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+            SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+            Region.getMCDCParams()));
       } else if (Region.isMCDCDecision()) {
         MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-            Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-            SR.LineEnd, SR.ColumnEnd));
+            Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+            SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
       } else {
         MappingRegions.push_back(CounterMappingRegion::makeRegion(
             Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
                     std::optional<SourceLocation> StartLoc = std::nullopt,
                     std::optional<SourceLocation> EndLoc = std::nullopt,
                     std::optional<Counter> FalseCount = std::nullopt,
-                    MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-                    MCDCConditionID FalseID = 0) {
+                    const MCDCParameters &BranchParams = std::monostate()) {
 
     if (StartLoc && !FalseCount) {
       MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
       StartLoc = std::nullopt;
     if (EndLoc && EndLoc->isInvalid())
       EndLoc = std::nullopt;
-    RegionStack.emplace_back(Count, FalseCount,
-                             MCDCParameters{0, 0, ID, TrueID, FalseID},
-                             StartLoc, EndLoc);
+    RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, EndLoc);
 
     return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
                     std::optional<SourceLocation> StartLoc = std::nullopt,
                     std::optional<SourceLocation> EndLoc = std::nullopt) {
 
-    RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
-                             EndLoc);
+    RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+                             StartLoc, EndLoc);
 
     return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
     // function's SourceRegions) because it doesn't apply to any other source
     // code other than the Condition.
     if (CodeGenFunction::isInstrumentedCondition(C)) {
+      MCDCParameters BranchParams;
       MCDCConditionID ID = MCDCBuilder.getCondID(C);
-      MCDCConditionID TrueID = IDPair.TrueID;
-      MCDCConditionID FalseID = IDPair.FalseID;
+      if (ID > 0)
+        BranchParams = MCDCBranchParameters{ID, IDPair.TrueID, IDPair.FalseID};
 
       // If a condition can fold to true or false, the corresponding branch
       // will be removed.  Create a region with both counters hard-coded to
@@ -1054,11 +1064,11 @@ struct CounterCoverageMappingBuilder
       // CodeGenFunction.c always returns false, but that is very heavy-handed.
       if (ConditionFoldsToBool(C))
         popRegions(pushRegion(Counter::getZero(), getStart(C), getEnd(C),
-                              Counter::getZero(), ID, TrueID, FalseID));
+                              Counter::getZero(), BranchParams));
       else
         // Otherwise, create a region with the True counter and False counter.
-        popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt, ID,
-                              TrueID, FalseID));
+        popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt,
+                              BranchParams));
     }
   }
 
@@ -1149,12 +1159,9 @@ struct CounterCoverageMappingBuilder
         // we've seen this region.
         if (StartLocs.insert(Loc).second) {
           if (I.isBranch())
-            SourceRegions.emplace_back(
-                I.getCounter(), I.getFalseCounter(),
-                MCDCParameters{0, 0, I.getMCDCParams().ID,
-                               I.getMCDCParams().TrueID,
-                               I.getMCDCParams().FalseID},
-                Loc, getEndOfFileOrMacro(Loc), I.isBranch());
+            SourceRegions.emplace_back(I.getCounter(), I.getFalseCounter(),
+                                       I.getMCDCParams(), Loc,
+                                       getEndOfFileOrMacro(Loc), I.isBranch());
           else
             SourceRegions.emplace_back(I.getCounter(), Loc,
                                        getEndOfFileOrMacro(Loc));
@@ -2120,9 +2127,10 @@ static void dump(llvm::raw_ostream &OS, StringRef FunctionName,
     OS << "File " << R.FileID << ", " << R.LineStart << ":" << R.ColumnStart
        << " -> " << R.LineEnd << ":" << R.ColumnEnd << " = ";
 
-    if (R.Kind == CounterMappingRegion::MCDCDecisionRegion) {
-      OS << "M:" << R.MCDCParams.BitmapIdx;
-      OS << ", C:" << R.MCDCParams.NumConditions;
+    if (const auto *DecisionParams =
+            std::get_if<MCDCDecisionParameters>(&R.MCDCParams)) {
+      OS << "M:" << DecisionParams->BitmapIdx;
+      OS << ", C:" << DecisionParams->NumConditions;
     } else {
       Ctx.dump(R.Count, OS);
 
@@ -2133,9 +2141,10 @@ static void dump(llvm::raw_ostream &OS, StringRef FunctionName,
       }
     }
 
-    if (R.Kind == CounterMappingRegion::MCDCBranchRegion) {
-      OS << " [" << R.MCDCParams.ID << "," << R.MCDCParams.TrueID;
-      OS << "," << R.MCDCParams.FalseID << "] ";
+    if (const auto *BranchParams =
+            std::get_if<MCDCBranchParameters>(&R.MCDCParams)) {
+      OS << " [" << BranchParams->ID << "," << BranchParams->TrueID;
+      OS << "," << BranchParams->FalseID << "] ";
     }
 
     if (R.Kind == CounterMappingRegion::ExpansionRegion)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 88ec60c7aa33c6..0ad6d48a0eb798 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -39,6 +39,7 @@
 #include <system_error>
 #include <tuple>
 #include <utility>
+#include <variant>
 #include <vector>
 
 namespace llvm {
@@ -250,18 +251,27 @@ struct CounterMappingRegion {
   };
 
   using MCDCConditionID = unsigned int;
-  struct MCDCParameters {
+  struct MCDCDecisionParameters {
     /// Byte Index of Bitmap Coverage Object for a Decision Region.
-    unsigned BitmapIdx = 0;
+    unsigned BitmapIdx;
 
     /// Number of Conditions used for a Decision Region.
-    unsigned NumConditions = 0;
+    unsigned NumConditions;
 
+    MCDCDecisionParameters() = delete;
+  };
+
+  struct MCDCBranchParameters {
     /// IDs used to represent a branch region and other branch regions
     /// evaluated based on True and False branches.
-    MCDCConditionID ID = 0, TrueID = 0, FalseID = 0;
+    MCDCConditionID ID, TrueID, FalseID;
+
+    MCDCBranchParameters() = delete;
   };
 
+  using MCDCParameters = std::variant<std::monostate, MCDCDecisionParameters,
+                                      MCDCBranchParameters>;
+
   /// Primary Counter that is also used for Branch Regions (TrueCount).
   Counter Count;
 
@@ -271,6 +281,24 @@ struct CounterMappingRegion {
   /// Parameters used for Modified Condition/Decision Coverage
   MCDCParameters MCDCParams;
 
+  template <class MaybeConstInnerParameters, class MaybeConstMCDCParameters>
+  static auto &getParams(MaybeConstMCDCParameters &MCDCParams) {
+    using InnerParameters =
+        typename std::remove_const<MaybeConstInnerParameters>::type;
+    MaybeConstInnerParameters *Params =
+        std::get_if<InnerParameters>(&MCDCParams);
+    assert(Params && "InnerParameters unavailable");
+    return *Params;
+  }
+
+  const auto &getDecisionParams() const {
+    return getParams<const MCDCDecisionParameters>(MCDCParams);
+  }
+
+  const auto &getBranchParams() const {
+    return getParams<const MCDCBranchParameters>(MCDCParams);
+  }
+
   unsigned FileID = 0;
   unsigned ExpandedFileID = 0;
   unsigned LineStart, ColumnStart, LineEnd, ColumnEnd;
@@ -284,19 +312,20 @@ struct CounterMappingRegion {
         LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd),
         ColumnEnd(ColumnEnd), Kind(Kind) {}
 
-  CounterMappingRegion(Counter Count, Counter FalseCount,
-                       MCDCParameters MCDCParams, unsigned FileID,
+  CounterMappingRegion(Counter Count, Counter FalseCount, unsigned FileID,
                        unsigned ExpandedFileID, unsigned LineStart,
                        unsigned ColumnStart, unsigned LineEnd,
-                       unsigned ColumnEnd, RegionKind Kind)
+                       unsigned ColumnEnd, RegionKind Kind,
+                       const MCDCParameters &MCDCParams = std::monostate())
       : Count(Count), FalseCount(FalseCount), MCDCParams(MCDCParams),
         FileID(FileID), ExpandedFileID(ExpandedFileID), LineStart(LineStart),
         ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd),
         Kind(Kind) {}
 
-  CounterMappingRegion(MCDCParameters MCDCParams, unsigned FileID,
-                       unsigned LineStart, unsigned ColumnStart,
-                       unsigned LineEnd, unsigned ColumnEnd, RegionKind Kind)
+  CounterMappingRegion(const MCDCDecisionParameters &MCDCParams,
+                       unsigned FileID, unsigned LineStart,
+                       unsigned ColumnStart, unsigned LineEnd,
+                       unsigned ColumnEnd, RegionKind Kind)
       : MCDCParams(MCDCParams), FileID(FileID), LineStart(LineStart),
         ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd),
         Kind(Kind) {}
@@ -333,24 +362,18 @@ struct CounterMappingRegion {
   static CounterMappingRegion
   makeBranchRegion(Counter Count, Counter FalseCount, unsigned FileID,
                    unsigned LineStart, unsigned ColumnStart, unsigned LineEnd,
-                   unsigned ColumnEnd) {
-    return CounterMappingRegion(Count, FalseCount, MCDCParameters(), FileID, 0,
-                                LineStart, ColumnStart, LineEnd, ColumnEnd,
-                                BranchRegion);
-  }
-
-  static CounterMappingRegion
-  makeBranchRegion(Counter Count, Counter FalseCount, MCDCParameters MCDCParams,
-                   unsigned FileID, unsigned LineStart, unsigned ColumnStart,
-                   unsigned LineEnd, unsigned ColumnEnd) {
-    return CounterMappingRegion(Count, FalseCount, MCDCParams, FileID, 0,
-                                LineStart, ColumnStart, LineEnd, ColumnEnd,
-                                MCDCParams.ID == 0 ? BranchRegion
-                                                   : MCDCBranchRegion);
+                   unsigned ColumnEnd,
+                   const MCDCParameters &MCDCParams = std::monostate()) {
+    return CounterMappingRegion(Count, FalseCount, FileID, 0, LineStart,
+                                ColumnStart, LineEnd, ColumnEnd,
+                                (std::get_if<MCDCBranchParameters>(&MCDCParams)
+                                     ? MCDCBranchRegion
+                                     : BranchRegion),
+                                MCDCParams);
   }
 
   static CounterMappingRegion
-  makeDecisionRegion(MCDCParameters MCDCParams, unsigned FileID,
+  makeDecisionRegion(const MCDCDecisionParameters &MCDCParams, unsigned FileID,
                      unsigned LineStart, unsigned ColumnStart, unsigned LineEnd,
                      unsigned ColumnEnd) {
     return CounterMappingRegion(MCDCParams, FileID, LineStart, ColumnStart,
@@ -415,9 +438,7 @@ struct MCDCRecord {
 
   CounterMappingRegion getDecisionRegion() const { return Region; }
   unsigned getNumConditions() const {
-    assert(Region.MCDCParams.NumConditions != 0 &&
-           "In MC/DC, NumConditions should never be zero!");
-    return Region.MCDCParams.NumConditions;
+    return Region.getDecisionParams().NumConditions;
   }
   unsigned getNumTestVectors() const { return TV.size(); }
   bool isCondFolded(unsigned Condition) const { return Folded[Condition]; }
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 6b189c31463283..646b9fb077ef07 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -234,6 +234,7 @@ class MCDCRecordProcessor {
 
   /// Decision Region to which the ExecutedTestVectorBitmap applies.
   const CounterMappingRegion &Region;
+  const CounterMappingRegion::MCDCDecisionParameters &DecisionParams;
 
   /// Array of branch regions corresponding each conditions in the boolean
   /// expression.
@@ -244,8 +245,9 @@ class MCDCRecordProcessor {
 
   unsigned BitmapIdx;
 
-  /// Mapping of a condition ID to its corresponding branch region.
-  llvm::DenseMap<unsigned, const CounterMappingRegion *> Map;
+  /// Mapping of a condition ID to its corresponding branch params.
+  llvm::DenseMap<unsigned, const CounterMappingRegion::MCDCBranchParameters *>
+      BranchParamsMap;
 
   /// Vector used to track whether a condition is constant folded.
   MCDCRecord::BoolVector Folded;
@@ -264,9 +266,10 @@ class MCDCRecordProcessor {
   MCDCRecordProcessor(const BitVector &Bitmap,
                       const CounterMappingRegion &Region,
                       ArrayRef<const CounterMappingRegion *> Branches)
-      : Bitmap(Bitmap), Region(Region), Branches(Branches),
-        NumConditions(Region.MCDCParams.NumConditions),
-        BitmapIdx(Region.MCDCParams.BitmapIdx * CHAR_BIT),
+      : Bitmap(Bitmap), Region(Region),
+        DecisionParams(Region.getDecisionParams()), Branches(Branches),
+        NumConditions(DecisionParams.NumConditions),
+        BitmapIdx(DecisionParams.BitmapIdx * CHAR_BIT),
         Folded(NumConditions, false), IndependencePairs(NumConditions),
         TestVectors((size_t)1 << NumConditions) {}
 
@@ -286,18 +289,18 @@ class MCDCRecordProcessor {
   // the truth table.
   void buildTestVector(MCDCRecord::TestVector &TV, unsigned ID,
                        unsigned Index) {
-    const CounterMappingRegion *Branch = Map[ID];
+    auto [UnusedID, TrueID, FalseID] = *BranchParamsMap[ID];
 
     TV[ID - 1] = MCDCRecord::MCDC_False;
-    if (Branch->MCDCParams.FalseID > 0)
-      buildTestVector(TV, Branch->MCDCParams.FalseID, Index);
+    if (FalseID > 0)
+      buildTestVector(TV, FalseID, Index);
     else
       recordTestVector(TV, Index, MCDCRecord::MCDC_False);
 
     Index |= 1 << (ID - 1);
     TV[ID - 1] = MCDCRecord::MCDC_True;
-    if (Branch->MCDCParams.TrueID > 0)
-      buildTestVector(TV, Branch->MCDCParams.TrueID, Index);
+    if (TrueID > 0)
+      buildTestVector(TV, TrueID, Index);
     else
       recordTestVector(TV, Index, MCDCRecord::MCDC_True);
 
@@ -374,8 +377,9 @@ class MCDCRecordProcessor {
     // - Record whether the condition is constant folded so that we exclude it
     //   from being measured.
     for (const auto *B : Branches) {
-      Map[B->MCDCParams.ID] = B;
-      PosToID[I] = B->MCDCParams.ID - 1;
+      const auto &BranchParams = B->getBranchParams();
+      BranchParamsMap[BranchParams.ID] = &BranchParams;
+      PosToID[I] = BranchParams.ID - 1;
       CondLoc[I] = B->startLoc();
       Folded[I++] = (B->Count.isZero() && B->FalseCount.isZero());
     }
@@ -501,10 +505,12 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx,
   // Note that `<=` is used insted of `<`, because `BitmapIdx == 0` is valid
   // and `MaxBitmapIdx is `unsigned`. `BitmapIdx` is unique in the record.
   for (const auto &Region : reverse(Record.MappingRegions)) {
-    if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion &&
-        MaxBitmapIdx <= Region.MCDCParams.BitmapIdx) {
-      MaxBitmapIdx = Region.MCDCParams.BitmapIdx;
-      NumConditions = Region.MCDCParams.NumConditions;
+    if (Region.Kind != CounterMappingRegion::MCDCDecisionRegion)
+      continue;
+    const auto &DecisionParams = Region.getDecisionParams();
+    if (MaxBitmapIdx <= DecisionParams.BitmapIdx) {
+      MaxBitmapIdx = DecisionParams.BitmapIdx;
+      NumConditions = DecisionParams.NumConditions;
     }
   }
   unsigned SizeInBits = llvm::alignTo(uint64_t(1) << NumConditions, CHAR_BIT);
@@ -526,6 +532,7 @@ class MCDCDecisionRecorder {
     /// They are reflected from DecisionRegion for convenience.
     LineColPair DecisionStartLoc;
     LineColPair DecisionEndLoc;
+    CounterMappingRegion::MCDCDecisionParameters DecisionParams;
 
     /// This is passed to `MCDCRecordProcessor`, so this should be compatible
     /// to`ArrayRef<const CounterMappingRegion *>`.
@@ -543,7 +550,8 @@ class MCDCDecisionRecorder {
 
     DecisionRecord(const CounterMappingRegion &Decision)
         : DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()),
-          DecisionEndLoc(Decision.endLoc()) {
+          DecisionEndLoc(Decision.endLoc()),
+          DecisionParams(Decision.getDecisionParams()) {
       assert(Decision.Kind == CounterMappingRegion::MCDCDecisionRegion);
     }
 
@@ -570,17 +578,17 @@ class MCDCDecisionRecorder {
     Result addBranch(const CounterMappingRegion &Branch) {
       assert(Branch.Kind == CounterMappingRegion::MCDCBranchRegion);
 
-      auto ConditionID = Branch.MCDCParams.ID;
+      auto ConditionID = Branch.getBranchParams().ID;
       assert(ConditionID > 0 && "ConditionID should begin with 1");
 
       if (ConditionIDs.contains(ConditionID) ||
-          ConditionID > DecisionRegion->MCDCParams.NumConditions)
+          ConditionID > DecisionParams.NumConditions)
         return NotProcessed;
 
       if (!this->dominates(Branch))
         return NotProcessed;
 
-      assert(MCDCBranches.size() < DecisionRegion->MCDCParams.NumConditions);
+      assert(MCDCBranches.size() < DecisionParams.NumConditions);
 
       // Put `ID=1` in front of `MCDCBranches` for convenience
       // even if `MCDCBranches` is not topological.
@@ -593,9 +601,8 @@ class MCDCDecisionRecorder {
       ConditionIDs.insert(ConditionID);
 
       // `Completed` when `MCDCBranches` is full
-      return (MCDCBranches.size() == DecisionRegion->MCDCParams.NumConditions
-                  ? Completed
-                  : Processed);
+      return (MCDCBranches.size() == DecisionParams.NumConditions ? Completed
+                                                                  : Processed);
     }
 
     /// Record Expansion if it is relevant to this Decision.
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index ac8e6b56379f21..bd2ca2afe31d6a 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -244,7 +244,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
   unsigned LineStart = 0;
   for (size_t I = 0; I < NumRegions; ++I) {
     Counter C, C2;
-    uint64_t BIDX = 0, NC = 0, ID = 0, TID = 0, FID = 0;
+    uint64_t BIDX, NC, ID, TID, FID;
+    CounterMappingRegion::MCDCParameters Params;
     CounterMappingRegion::RegionKind Kind = CounterMappingRegion::CodeRegion;
 
     // Read the combined counter + region kind.
@@ -308,6 +309,9 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
             return Err;
           if (auto Err = readIntMax(FID, std::numeric_limits<unsigned>::max()))
             return Err;
+          assert(ID > 0);
+          Params = CounterMappingRegion::MCDCBranchParameters{
+              (unsigned)ID, (unsigned)TID, (unsigned)FID};
           break;
         case CounterMappingRegion::MCDCDecisionRegion:
           Kind = CounterMappingRegion::MCDCDecisionRegion;
@@ -315,6 +319,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
             return Err;
           if (auto Err = readIntMax(NC, std::numeric_limits<unsigned>::max()))
             return Err;
+          Params = CounterMappingRegion::MCDCDecisionParameters{(unsigned)BIDX,
+                                                                (unsigned)NC};
           break;
         default:
           return make_error<CoverageMapError>(coveragemap_error::malformed,
@@ -370,13 +376,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
     });
 
     auto CMR = CounterMappingRegion(
-        C, C2,
-        CounterMappingRegion::MCDCParameters{
-            static_cast<unsigned>(BIDX), static_cast<unsigned>(NC),
-            static_cast<unsigned>(ID), static_cast<unsigned>(TID),
-            static_cast<unsigned>(FID)},
-        InferredFileID, ExpandedFileID, LineStart, ColumnStart,
-        LineStart + NumLines, ColumnEnd, Kind);
+        C, C2, InferredFileID, ExpandedFileID, LineStart, ColumnStart,
+        LineStart + NumLines, ColumnEnd, Kind, Params);
     if (CMR.startLoc() > CMR.endLoc())
       return make_error<CoverageMapError>(
           coveragemap_error::malformed,
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 27727f216b0513..8706f8e677bce6 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -213,6 +213,7 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
     }
     Counter Count = Minimizer.adjust(I->Count);
     Counter FalseCount = Minimizer.adjust(I->FalseCount);
+    bool ParamsShouldBeNull = true;
     switch (I->Kind) {
     case CounterMappingRegion::CodeRegion:
     case CounterMappingRegion::GapRegion:
@@ -251,16 +252,25 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
                     OS);
       writeCounter(MinExpressions, Count, OS);
       writeCounter(MinExpressions, FalseCount, OS);
-      encodeULEB128(unsigned(I->MCDCParams.ID), OS);
-      encodeULEB128(unsigned(I->MCDCParams.TrueID), OS);
-      encodeULEB128(unsigned(I->MCDCParams.FalseID), OS);
+      {
+        const auto &BranchParams = I->getBranchParams();
+        ParamsShouldBeNull = false;
+        assert(BranchParams.ID > 0);
+        encodeULEB128(unsigned(BranchParams.ID), OS);
+        encodeULEB128(unsigned(BranchParams.TrueID), OS);
+        encodeULEB128(unsigned(BranchParams.FalseID), OS);
+      }
       break;
     case CounterMappingRegion::MCDCDecisionRegion:
       encodeULEB128(unsigned(I->Kind)
                         << Counter::EncodingCounterTagAndExpansionRegionTagBits,
                     OS);
-      encodeULEB128(unsigned(I->MCDCParams.BitmapIdx), OS);
-      encodeULEB128(unsigned(I->MCDCParams.NumConditions), OS);
+      {
+        const auto &DecisionParams = I->getDecisionParams();
+        ParamsShouldBeNull = false;
+        encodeULEB128(unsigned(DecisionParams.BitmapIdx), OS);
+        encodeULEB128(unsigned(DecisionParams.NumConditions), OS);
+      }
       break;
     }
     assert(I->LineStart >= PrevLineStart);
@@ -270,6 +280,9 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
     encodeULEB128(I->LineEnd - I->LineStart, OS);
     encodeULEB128(I->ColumnEnd, OS);
     PrevLineStart = I->LineStart;
+    assert((!ParamsShouldBeNull || std::get_if<0>(&I->MCDCParams)) &&
+           "MCDCParams should be empty");
+    (void)ParamsShouldBeNull;
   }
   // Ensure that all file ids have at least one mapping region.
   assert(CurrentFileID == (VirtualFileMapping.size() - 1));
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index 2849781a9dc43b..836afa7bd1b129 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -197,8 +197,8 @@ struct CoverageMappingTest : ::testing::TestWithParam<std::tuple<bool, bool>> {
     auto &Regions = InputFunctions.back().Regions;
     unsigned FileID = getFileIndexForFunction(File);
     Regions.push_back(CounterMappingRegion::makeDecisionRegion(
-        CounterMappingRegion::MCDCParameters{Mask, NC}, FileID, LS, CS, LE,
-        CE));
+        CounterMappingRegion::MCDCDecisionParameters{Mask, NC}, FileID, LS, CS,
+        LE, CE));
   }
 
   void addMCDCBranchCMR(Counter C1, Counter C2, unsigned ID, unsigned TrueID,
@@ -207,8 +207,8 @@ struct CoverageMappingTest : ::testing::TestWithParam<std::tuple<bool, bool>> {
     auto &Regions = InputFunctions.back().Regions;
     unsigned FileID = getFileIndexForFunction(File);
     Regions.push_back(CounterMappingRegion::makeBranchRegion(
-        C1, C2, CounterMappingRegion::MCDCParameters{0, 0, ID, TrueID, FalseID},
-        FileID, LS, CS, LE, CE));
+        C1, C2, FileID, LS, CS, LE, CE,
+        CounterMappingRegion::MCDCBranchParameters{ID, TrueID, FalseID}));
   }
 
   void addExpansionCMR(StringRef File, StringRef ExpandedFile, unsigned LS,

>From 7f4af8de53089cbe4c7e62ba6f2cd6199d048c77 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Sun, 11 Feb 2024 01:43:46 +0900
Subject: [PATCH 2/3] CoverageMappingReader: MCDCConditionID shouldn't be zero

---
 llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index bd2ca2afe31d6a..598590db7c39ae 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -309,7 +309,10 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
             return Err;
           if (auto Err = readIntMax(FID, std::numeric_limits<unsigned>::max()))
             return Err;
-          assert(ID > 0);
+          if (ID == 0)
+            return make_error<CoverageMapError>(
+                coveragemap_error::malformed,
+                "MCDCConditionID shouldn't be zero");
           Params = CounterMappingRegion::MCDCBranchParameters{
               (unsigned)ID, (unsigned)TID, (unsigned)FID};
           break;

>From 109d273fe5b06683c20e902844600b81871441a3 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Mon, 12 Feb 2024 18:08:23 +0900
Subject: [PATCH 3/3] static_cast

---
 llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index 598590db7c39ae..670fa4a7aeda55 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -314,7 +314,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
                 coveragemap_error::malformed,
                 "MCDCConditionID shouldn't be zero");
           Params = CounterMappingRegion::MCDCBranchParameters{
-              (unsigned)ID, (unsigned)TID, (unsigned)FID};
+              static_cast<unsigned>(ID), static_cast<unsigned>(TID),
+              static_cast<unsigned>(FID)};
           break;
         case CounterMappingRegion::MCDCDecisionRegion:
           Kind = CounterMappingRegion::MCDCDecisionRegion;
@@ -322,8 +323,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
             return Err;
           if (auto Err = readIntMax(NC, std::numeric_limits<unsigned>::max()))
             return Err;
-          Params = CounterMappingRegion::MCDCDecisionParameters{(unsigned)BIDX,
-                                                                (unsigned)NC};
+          Params = CounterMappingRegion::MCDCDecisionParameters{
+              static_cast<unsigned>(BIDX), static_cast<unsigned>(NC)};
           break;
         default:
           return make_error<CoverageMapError>(coveragemap_error::malformed,



More information about the cfe-commits mailing list