[MC/DC] Refactor: Introduce ConditionIDs as std::array<2> (#81221)

chapuni · web-flow · commit 1a1fcacbce80 · 2024-02-14T23:17:00.000+09:00
Its 0th element corresponds to `FalseID` and 1st to `TrueID`.

CoverageMappingGen.cpp: `DecisionIDPair` is replaced with `ConditionIDs`
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -593,11 +593,6 @@ struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {
 /// creation.
 struct MCDCCoverageBuilder {
 
-  struct DecisionIDPair {
-    mcdc::ConditionID TrueID = 0;
-    mcdc::ConditionID FalseID = 0;
-  };
-
   /// The AST walk recursively visits nested logical-AND or logical-OR binary
   /// operator nodes and then visits their LHS and RHS children nodes.  As this
   /// happens, the algorithm will assign IDs to each operator's LHS and RHS side
@@ -688,14 +683,14 @@ struct MCDCCoverageBuilder {
 private:
   CodeGenModule &CGM;
 
-  llvm::SmallVector<DecisionIDPair> DecisionStack;
+  llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
   MCDC::State &MCDCState;
   llvm::DenseMap<const Stmt *, mcdc::ConditionID> &CondIDs;
   mcdc::ConditionID NextID = 1;
   bool NotMapped = false;
 
   /// Represent a sentinel value of [0,0] for the bottom of DecisionStack.
-  static constexpr DecisionIDPair DecisionStackSentinel{0, 0};
+  static constexpr mcdc::ConditionIDs DecisionStackSentinel{0, 0};
 
   /// Is this a logical-AND operation?
   bool isLAnd(const BinaryOperator *E) const {
@@ -732,7 +727,7 @@ struct MCDCCoverageBuilder {
   }
 
   /// Return the LHS Decision ([0,0] if not set).
-  const DecisionIDPair &back() const { return DecisionStack.back(); }
+  const mcdc::ConditionIDs &back() const { return DecisionStack.back(); }
 
   /// Push the binary operator statement to track the nest level and assign IDs
   /// to the operator's LHS and RHS.  The RHS may be a larger subtree that is
@@ -750,7 +745,7 @@ struct MCDCCoverageBuilder {
     if (NotMapped)
       return;
 
-    const DecisionIDPair &ParentDecision = DecisionStack.back();
+    const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();
 
     // If the operator itself has an assigned ID, this means it represents a
     // larger subtree.  In this case, assign that ID to its LHS node.  Its RHS
@@ -766,18 +761,18 @@ struct MCDCCoverageBuilder {
 
     // Push the LHS decision IDs onto the DecisionStack.
     if (isLAnd(E))
-      DecisionStack.push_back({RHSid, ParentDecision.FalseID});
+      DecisionStack.push_back({ParentDecision[false], RHSid});
     else
-      DecisionStack.push_back({ParentDecision.TrueID, RHSid});
+      DecisionStack.push_back({RHSid, ParentDecision[true]});
   }
 
   /// Pop and return the LHS Decision ([0,0] if not set).
-  DecisionIDPair pop() {
+  mcdc::ConditionIDs pop() {
     if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
       return DecisionStack.front();
 
     assert(DecisionStack.size() > 1);
-    DecisionIDPair D = DecisionStack.back();
+    mcdc::ConditionIDs D = DecisionStack.back();
     DecisionStack.pop_back();
     return D;
   }
@@ -1026,15 +1021,12 @@ struct CounterCoverageMappingBuilder
     return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
   }
 
-  using MCDCDecisionIDPair = MCDCCoverageBuilder::DecisionIDPair;
-
   /// Create a Branch Region around an instrumentable condition for coverage
   /// and add it to the function's SourceRegions.  A branch region tracks a
   /// "True" counter and a "False" counter for boolean expressions that
   /// result in the generation of a branch.
-  void
-  createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
-                     const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {
+  void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
+                          const mcdc::ConditionIDs &Conds = {}) {
     // Check for NULL conditions.
     if (!C)
       return;
@@ -1047,8 +1039,7 @@ struct CounterCoverageMappingBuilder
       mcdc::Parameters BranchParams;
       mcdc::ConditionID ID = MCDCBuilder.getCondID(C);
       if (ID > 0)
-        BranchParams =
-            mcdc::BranchParameters{ID, IDPair.TrueID, IDPair.FalseID};
+        BranchParams = mcdc::BranchParameters{ID, Conds};
 
       // If a condition can fold to true or false, the corresponding branch
       // will be removed.  Create a region with both counters hard-coded to
@@ -2134,8 +2125,8 @@ static void dump(llvm::raw_ostream &OS, StringRef FunctionName,
 
     if (const auto *BranchParams =
             std::get_if<mcdc::BranchParameters>(&R.MCDCParams)) {
-      OS << " [" << BranchParams->ID << "," << BranchParams->TrueID;
-      OS << "," << BranchParams->FalseID << "] ";
+      OS << " [" << BranchParams->ID << "," << BranchParams->Conds[true];
+      OS << "," << BranchParams->Conds[false] << "] ";
     }
 
     if (R.Kind == CounterMappingRegion::ExpansionRegion)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -38,7 +38,6 @@
 #include <sstream>
 #include <string>
 #include <system_error>
-#include <tuple>
 #include <utility>
 #include <vector>
 
diff --git a/llvm/include/llvm/ProfileData/Coverage/MCDCTypes.h b/llvm/include/llvm/ProfileData/Coverage/MCDCTypes.h
@@ -13,12 +13,14 @@
 #ifndef LLVM_PROFILEDATA_COVERAGE_MCDCTYPES_H
 #define LLVM_PROFILEDATA_COVERAGE_MCDCTYPES_H
 
+#include <array>
 #include <variant>
 
 namespace llvm::coverage::mcdc {
 
 /// The ID for MCDCBranch.
 using ConditionID = unsigned int;
+using ConditionIDs = std::array<ConditionID, 2>;
 
 struct DecisionParameters {
   /// Byte Index of Bitmap Coverage Object for a Decision Region.
@@ -35,11 +37,12 @@ struct DecisionParameters {
 struct BranchParameters {
   /// IDs used to represent a branch region and other branch regions
   /// evaluated based on True and False branches.
-  ConditionID ID, TrueID, FalseID;
+  ConditionID ID;
+  ConditionIDs Conds;
 
   BranchParameters() = delete;
-  BranchParameters(ConditionID ID, ConditionID TrueID, ConditionID FalseID)
-      : ID(ID), TrueID(TrueID), FalseID(FalseID) {}
+  BranchParameters(ConditionID ID, const ConditionIDs &Conds)
+      : ID(ID), Conds(Conds) {}
 };
 
 /// The type of MC/DC-specific parameters.
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -246,7 +246,7 @@ class MCDCRecordProcessor {
   unsigned BitmapIdx;
 
   /// Mapping of a condition ID to its corresponding branch params.
-  llvm::DenseMap<unsigned, const mcdc::BranchParameters *> BranchParamsMap;
+  llvm::DenseMap<unsigned, mcdc::ConditionIDs> CondsMap;
 
   /// Vector used to track whether a condition is constant folded.
   MCDCRecord::BoolVector Folded;
@@ -269,38 +269,34 @@ class MCDCRecordProcessor {
         Folded(NumConditions, false), IndependencePairs(NumConditions) {}
 
 private:
-  void recordTestVector(MCDCRecord::TestVector &TV, unsigned Index,
-                        MCDCRecord::CondState Result) {
-    if (!Bitmap[BitmapIdx + Index])
-      return;
-
-    // Copy the completed test vector to the vector of testvectors.
-    ExecVectors.push_back(TV);
-
-    // The final value (T,F) is equal to the last non-dontcare state on the
-    // path (in a short-circuiting system).
-    ExecVectors.back().push_back(Result);
-  }
-
   // Walk the binary decision diagram and try assigning both false and true to
   // each node. When a terminal node (ID == 0) is reached, fill in the value in
   // the truth table.
   void buildTestVector(MCDCRecord::TestVector &TV, unsigned ID,
                        unsigned Index) {
-    auto [UnusedID, TrueID, FalseID] = *BranchParamsMap[ID];
+    assert((Index & (1 << (ID - 1))) == 0);
+
+    for (auto MCDCCond : {MCDCRecord::MCDC_False, MCDCRecord::MCDC_True}) {
+      static_assert(MCDCRecord::MCDC_False == 0);
+      static_assert(MCDCRecord::MCDC_True == 1);
+      Index |= MCDCCond << (ID - 1);
+      TV[ID - 1] = MCDCCond;
+      auto NextID = CondsMap[ID][MCDCCond];
+      if (NextID > 0) {
+        buildTestVector(TV, NextID, Index);
+        continue;
+      }
 
-    TV[ID - 1] = MCDCRecord::MCDC_False;
-    if (FalseID > 0)
-      buildTestVector(TV, FalseID, Index);
-    else
-      recordTestVector(TV, Index, MCDCRecord::MCDC_False);
+      if (!Bitmap[BitmapIdx + Index])
+        continue;
 
-    Index |= 1 << (ID - 1);
-    TV[ID - 1] = MCDCRecord::MCDC_True;
-    if (TrueID > 0)
-      buildTestVector(TV, TrueID, Index);
-    else
-      recordTestVector(TV, Index, MCDCRecord::MCDC_True);
+      // Copy the completed test vector to the vector of testvectors.
+      ExecVectors.push_back(TV);
+
+      // The final value (T,F) is equal to the last non-dontcare state on the
+      // path (in a short-circuiting system).
+      ExecVectors.back().push_back(MCDCCond);
+    }
 
     // Reset back to DontCare.
     TV[ID - 1] = MCDCRecord::MCDC_DontCare;
@@ -374,7 +370,7 @@ class MCDCRecordProcessor {
     //   from being measured.
     for (const auto *B : Branches) {
       const auto &BranchParams = B->getBranchParams();
-      BranchParamsMap[BranchParams.ID] = &BranchParams;
+      CondsMap[BranchParams.ID] = BranchParams.Conds;
       PosToID[I] = BranchParams.ID - 1;
       CondLoc[I] = B->startLoc();
       Folded[I++] = (B->Count.isZero() && B->FalseCount.isZero());
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -313,9 +313,9 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
             return make_error<CoverageMapError>(
                 coveragemap_error::malformed,
                 "MCDCConditionID shouldn't be zero");
-          Params = mcdc::BranchParameters{static_cast<unsigned>(ID),
-                                          static_cast<unsigned>(TID),
-                                          static_cast<unsigned>(FID)};
+          Params = mcdc::BranchParameters{
+              static_cast<unsigned>(ID),
+              {static_cast<unsigned>(FID), static_cast<unsigned>(TID)}};
           break;
         case CounterMappingRegion::MCDCDecisionRegion:
           Kind = CounterMappingRegion::MCDCDecisionRegion;
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -257,8 +257,8 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
         ParamsShouldBeNull = false;
         assert(BranchParams.ID > 0);
         encodeULEB128(static_cast<unsigned>(BranchParams.ID), OS);
-        encodeULEB128(static_cast<unsigned>(BranchParams.TrueID), OS);
-        encodeULEB128(static_cast<unsigned>(BranchParams.FalseID), OS);
+        encodeULEB128(static_cast<unsigned>(BranchParams.Conds[true]), OS);
+        encodeULEB128(static_cast<unsigned>(BranchParams.Conds[false]), OS);
       }
       break;
     case CounterMappingRegion::MCDCDecisionRegion:
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -200,14 +200,13 @@ struct CoverageMappingTest : ::testing::TestWithParam<std::tuple<bool, bool>> {
         mcdc::DecisionParameters{Mask, NC}, FileID, LS, CS, LE, CE));
   }
 
-  void addMCDCBranchCMR(Counter C1, Counter C2, unsigned ID, unsigned TrueID,
-                        unsigned FalseID, StringRef File, unsigned LS,
+  void addMCDCBranchCMR(Counter C1, Counter C2, mcdc::ConditionID ID,
+                        mcdc::ConditionIDs Conds, StringRef File, unsigned LS,
                         unsigned CS, unsigned LE, unsigned CE) {
     auto &Regions = InputFunctions.back().Regions;
     unsigned FileID = getFileIndexForFunction(File);
     Regions.push_back(CounterMappingRegion::makeBranchRegion(
-        C1, C2, FileID, LS, CS, LE, CE,
-        mcdc::BranchParameters{ID, TrueID, FalseID}));
+        C1, C2, FileID, LS, CS, LE, CE, mcdc::BranchParameters{ID, Conds}));
   }
 
   void addExpansionCMR(StringRef File, StringRef ExpandedFile, unsigned LS,
@@ -873,9 +872,9 @@ TEST_P(CoverageMappingTest, non_code_region_bitmask) {
   addCMR(Counter::getCounter(3), "file", 1, 1, 5, 5);
 
   addMCDCDecisionCMR(0, 2, "file", 7, 1, 7, 6);
-  addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, 2, 0,
+  addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, {0, 2},
                    "file", 7, 2, 7, 3);
-  addMCDCBranchCMR(Counter::getCounter(2), Counter::getCounter(3), 2, 0, 0,
+  addMCDCBranchCMR(Counter::getCounter(2), Counter::getCounter(3), 2, {0, 0},
                    "file", 7, 4, 7, 5);
 
   EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
@@ -901,11 +900,11 @@ TEST_P(CoverageMappingTest, decision_before_expansion) {
   addExpansionCMR("foo", "B", 4, 19, 4, 20);
   addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
   addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
-  addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, 2, 0, "A",
-                   1, 14, 1, 17);
+  addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, {0, 2},
+                   "A", 1, 14, 1, 17);
   addCMR(Counter::getCounter(1), "B", 1, 14, 1, 17);
-  addMCDCBranchCMR(Counter::getCounter(1), Counter::getCounter(2), 2, 0, 0, "B",
-                   1, 14, 1, 17);
+  addMCDCBranchCMR(Counter::getCounter(1), Counter::getCounter(2), 2, {0, 0},
+                   "B", 1, 14, 1, 17);
 
   // InputFunctionCoverageData::Regions is rewritten after the write.
   auto InputRegions = InputFunctions.back().Regions;
