[MC/DC][Coverage] Loosen the limit of NumConds from 6

clang/lib/CodeGen/CodeGenPGO.cpp

@@ -163,8 +163,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
   PGOHash Hash;
   /// The map of statements to counters.
   llvm::DenseMap<const Stmt *, unsigned> &CounterMap;
-  /// The next bitmap byte index to assign.
-  unsigned NextMCDCBitmapIdx;
+  /// The state of MC/DC Coverage in this function.
   MCDC::State &MCDCState;
   /// Maximum number of supported MC/DC conditions in a boolean expression.
   unsigned MCDCMaxCond;
@@ -178,7 +177,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
                     MCDC::State &MCDCState, unsigned MCDCMaxCond,
                     DiagnosticsEngine &Diag)
       : NextCounter(0), Hash(HashVersion), CounterMap(CounterMap),
-        NextMCDCBitmapIdx(0), MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
+        MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
         ProfileVersion(ProfileVersion), Diag(Diag) {}
 
   // Blocks and lambdas are handled as separate functions, so we need not
@@ -306,11 +305,8 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
             return false;
           }
 
-          // Otherwise, allocate the number of bytes required for the bitmap
-          // based on the number of conditions. Must be at least 1-byte long.
-          MCDCState.BitmapMap[BinOp] = NextMCDCBitmapIdx;
-          unsigned SizeInBits = std::max<unsigned>(1L << NumCond, CHAR_BIT);
-          NextMCDCBitmapIdx += SizeInBits / CHAR_BIT;
+          // Otherwise, allocate the Decision.
+          MCDCState.BitmapMap[BinOp].BitmapIdx = 0;
         }
         return true;
       }
@@ -983,7 +979,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
   // for most embedded applications. Setting a maximum value prevents the
   // bitmap footprint from growing too large without the user's knowledge. In
   // the future, this value could be adjusted with a command-line option.
-  unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
+  unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 32767 : 0;
 
   RegionCounterMap.reset(new llvm::DenseMap<const Stmt *, unsigned>);
   RegionMCDCState.reset(new MCDC::State);
@@ -999,7 +995,6 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
     Walker.TraverseDecl(const_cast<CapturedDecl *>(CD));
   assert(Walker.NextCounter > 0 && "no entry counter mapped for decl");
   NumRegionCounters = Walker.NextCounter;
-  RegionMCDCState->BitmapBytes = Walker.NextMCDCBitmapIdx;
   FunctionHash = Walker.Hash.finalize();
 }
 
@@ -1124,9 +1119,11 @@ void CodeGenPGO::emitMCDCParameters(CGBuilderTy &Builder) {
   // Emit intrinsic representing MCDC bitmap parameters at function entry.
   // This is used by the instrumentation pass, but it isn't actually lowered to
   // anything.
-  llvm::Value *Args[3] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
-                          Builder.getInt64(FunctionHash),
-                          Builder.getInt32(RegionMCDCState->BitmapBytes)};
+  llvm::Value *Args[3] = {
+      llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
+      Builder.getInt64(FunctionHash),
+      Builder.getInt32(llvm::alignTo(RegionMCDCState->BitmapBits, CHAR_BIT) /
+                       CHAR_BIT)};
   Builder.CreateCall(
       CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_parameters), Args);
 }
@@ -1139,12 +1136,9 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
 
   S = S->IgnoreParens();
 
-  auto ExprMCDCBitmapMapIterator = RegionMCDCState->BitmapMap.find(S);
-  if (ExprMCDCBitmapMapIterator == RegionMCDCState->BitmapMap.end())
+  if (!RegionMCDCState->BitmapMap.contains(S))
     return;
 
-  // Extract the ID of the global bitmap associated with this expression.
-  unsigned MCDCTestVectorBitmapID = ExprMCDCBitmapMapIterator->second;
   auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
 
   // Emit intrinsic responsible for updating the global bitmap corresponding to
@@ -1154,9 +1148,8 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
   // index represents an executed test vector.
   llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
                           Builder.getInt64(FunctionHash),
-                          Builder.getInt32(RegionMCDCState->BitmapBytes),
-                          Builder.getInt32(MCDCTestVectorBitmapID),
-                          MCDCCondBitmapAddr.getPointer()};
+                          Builder.getInt32(RegionMCDCState->BitmapBits),
+                          Builder.getInt32(0), MCDCCondBitmapAddr.getPointer()};
   Builder.CreateCall(
       CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_tvbitmap_update), Args);
 }
@@ -1195,21 +1188,23 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
     return;
 
   // Extract the ID of the condition we are setting in the bitmap.
-  auto CondID = ExprMCDCConditionIDMapIterator->second;
-  assert(CondID >= 0 && "Condition has no ID!");
+  const auto &Branch = ExprMCDCConditionIDMapIterator->second;
+  assert(Branch.ID >= 0 && "Condition has no ID!");
 
-  auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
+  // Cancel the emission if the Decision is erased after the allocation.
+  const auto DecisionIter =
+      RegionMCDCState->BitmapMap.find(Branch.DecisionStmt);
+  if (DecisionIter == RegionMCDCState->BitmapMap.end())
+    return;
 
-  // Emit intrinsic that updates a dedicated temporary value on the stack after
-  // a condition is evaluated. After the set of conditions has been updated,
-  // the resulting value is used to update the boolean expression's bitmap.
-  llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
-                          Builder.getInt64(FunctionHash),
-                          Builder.getInt32(CondID),
-                          MCDCCondBitmapAddr.getPointer(), Val};
-  Builder.CreateCall(
-      CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_condbitmap_update),
-      Args);
+  const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
+
+  auto *CurTV = Builder.CreateLoad(MCDCCondBitmapAddr,
+                                   "mcdc." + Twine(Branch.ID + 1) + ".cur");
+  auto *NewTV = Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[true]));
+  NewTV = Builder.CreateSelect(
+      Val, NewTV, Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[false])));
+  Builder.CreateStore(NewTV, MCDCCondBitmapAddr);
 }
 
 void CodeGenPGO::setValueProfilingFlag(llvm::Module &M) {

clang/lib/CodeGen/CoverageMappingGen.cpp

@@ -182,6 +182,16 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
+  bool isMCDCBranch() const {
+    const auto *BranchParams = std::get_if<mcdc::BranchParameters>(&MCDCParams);
+    assert(!BranchParams || BranchParams->ID >= 0);
+    return BranchParams;
+  }
+
+  const auto &getMCDCBranchParams() const {
+    return mcdc::getParams<const mcdc::BranchParameters>(MCDCParams);
+  }
+
   bool isMCDCDecision() const {
     const auto *DecisionParams =
         std::get_if<mcdc::DecisionParameters>(&MCDCParams);
@@ -194,6 +204,8 @@ class SourceMappingRegion {
   }
 
   const mcdc::Parameters &getMCDCParams() const { return MCDCParams; }
+
+  void resetMCDCParams() { MCDCParams = mcdc::Parameters(); }
 };
 
 /// Spelling locations for the start and end of a source region.
@@ -684,7 +696,9 @@ struct MCDCCoverageBuilder {
 
   llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
   MCDC::State &MCDCState;
-  llvm::DenseMap<const Stmt *, mcdc::ConditionID> &CondIDs;
+  llvm::DenseMap<const Stmt *, MCDC::State::Branch> &CondIDs;
+  const Stmt *DecisionStmt = nullptr;
+  llvm::DenseMap<mcdc::ConditionID, mcdc::ConditionIDs> cache;
   mcdc::ConditionID NextID = 0;
   bool NotMapped = false;
 
@@ -714,7 +728,7 @@ struct MCDCCoverageBuilder {
 
   /// Set the given condition's ID.
   void setCondID(const Expr *Cond, mcdc::ConditionID ID) {
-    CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+    CondIDs[CodeGenFunction::stripCond(Cond)] = {ID, DecisionStmt};
   }
 
   /// Return the ID of a given condition.
@@ -723,7 +737,15 @@ struct MCDCCoverageBuilder {
     if (I == CondIDs.end())
       return -1;
     else
-      return I->second;
+      return I->second.ID;
+  }
+
+  void ccc(const Expr *CondExpr, mcdc::ConditionIDs IDs) {
+    auto ID = getCondID(CondExpr);
+    if (ID < 0)
+      return;
+    if (!cache.contains(ID))
+      cache[ID] = IDs;
   }
 
   /// Return the LHS Decision ([0,0] if not set).
@@ -745,6 +767,12 @@ struct MCDCCoverageBuilder {
     if (NotMapped)
       return;
 
+    if (NextID == 0) {
+      cache.clear();
+      DecisionStmt = E;
+      assert(MCDCState.BitmapMap.contains(E));
+    }
+
     const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();
 
     // If the operator itself has an assigned ID, this means it represents a
@@ -794,9 +822,33 @@ struct MCDCCoverageBuilder {
     // Set number of conditions and reset.
     unsigned TotalConds = NextID;
 
+    llvm::SmallVector<mcdc::ConditionIDs> R(cache.size());
+    for (auto [ID, Conds] : cache)
+      R[ID] = Conds;
+    mcdc::TVIdxBuilder xxx(R, MCDCState.BitmapBits);
+    auto NumTVs = xxx.NumTestVectors;
+    assert(MCDCState.BitmapMap.contains(E));
+    auto MaxTVs = mcdc::TVIdxBuilder::HardMaxTVs;
+    if (NumTVs == MaxTVs) {
+      auto &Diag = CGM.getDiags();
+      unsigned DiagID =
+          Diag.getCustomDiagID(DiagnosticsEngine::Warning,
+                               "unsupported MC/DC boolean expression; "
+                               "number of test vectors (%0) exceeds max (%1). "
+                               "Expression will not be covered");
+      Diag.Report(E->getBeginLoc(), DiagID) << NumTVs << MaxTVs;
+
+      MCDCState.BitmapMap[E].BitmapIdx = 0; // Mark to be erased
+    } else {
+      // Last pos
+      MCDCState.BitmapMap[E] = {MCDCState.BitmapBits += NumTVs,
+                                std::move(xxx.Indices)};
+    }
+
     // Reset ID back to beginning.
     NextID = 0;
 
+    // Return the number even if the Decision will be erased.
     return TotalConds;
   }
 };
@@ -853,7 +905,9 @@ struct CounterCoverageMappingBuilder
     return Counter::getCounter(CounterMap[S]);
   }
 
-  unsigned getRegionBitmap(const Stmt *S) { return MCDCState.BitmapMap[S]; }
+  auto getRegionBitmap(const Stmt *S) {
+    return MCDCState.BitmapMap[S].BitmapIdx;
+  }
 
   /// Push a region onto the stack.
   ///
@@ -1959,9 +2013,44 @@ struct CounterCoverageMappingBuilder
                        subtractCounters(ParentCount, TrueCount));
   }
 
+  void RewindDecision(unsigned Since) {
+#ifndef NDEBUG
+    llvm::DenseSet<mcdc::ConditionID> SeenIDs;
+#endif
+    unsigned NConds = 0;
+
+    assert(Since <= SourceRegions.size());
+    auto I = SourceRegions.begin() + Since;
+    while (I != SourceRegions.end()) {
+      if (I->isMCDCDecision()) {
+        assert(I->getMCDCDecisionParams().BitmapIdx == 0 &&
+               "It should be valid");
+        assert(NConds == 0 && "Duplicate MCDCDecision");
+        NConds = I->getMCDCDecisionParams().NumConditions;
+        assert(NConds > 0 && "Malformed MCDCDecision");
+        I = SourceRegions.erase(I);
+        continue;
+      }
+
+      if (I->isMCDCBranch()) {
+        assert(SeenIDs.insert(I->getMCDCBranchParams().ID).second &&
+               "Duplicate CondID");
+        I->resetMCDCParams();
+      }
+
+      ++I;
+    }
+
+    assert(NConds > 0 && "MCDCDecision wasn't found");
+    assert(SeenIDs.size() == NConds && "Unexpected number of MCDCBranch(es)");
+    (void)NConds;
+  }
+
   void VisitBinLAnd(const BinaryOperator *E) {
     bool IsRootNode = MCDCBuilder.isIdle();
 
+    unsigned SourceRegionsSince = SourceRegions.size();
+
     // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
@@ -1979,6 +2068,9 @@ struct CounterCoverageMappingBuilder
     // Track RHS True/False Decision.
     const auto DecisionRHS = MCDCBuilder.back();
 
+    MCDCBuilder.ccc(E->getLHS(), DecisionLHS);
+    MCDCBuilder.ccc(E->getRHS(), DecisionRHS);
+
     // Create MCDC Decision Region if at top-level (root).
     unsigned NumConds = 0;
     if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
@@ -2000,6 +2092,11 @@ struct CounterCoverageMappingBuilder
     // Create Branch Region around RHS condition.
     createBranchRegion(E->getRHS(), RHSTrueCnt,
                        subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS);
+
+    if (IsRootNode && NumConds > 0 && getRegionBitmap(E) == 0) {
+      RewindDecision(SourceRegionsSince);
+      MCDCState.BitmapMap.erase(E);
+    }
   }
 
   // Determine whether the right side of OR operation need to be visited.
@@ -2015,6 +2112,8 @@ struct CounterCoverageMappingBuilder
   void VisitBinLOr(const BinaryOperator *E) {
     bool IsRootNode = MCDCBuilder.isIdle();
 
+    unsigned SourceRegionsSince = SourceRegions.size();
+
     // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
@@ -2032,6 +2131,9 @@ struct CounterCoverageMappingBuilder
     // Track RHS True/False Decision.
     const auto DecisionRHS = MCDCBuilder.back();
 
+    MCDCBuilder.ccc(E->getLHS(), DecisionLHS);
+    MCDCBuilder.ccc(E->getRHS(), DecisionRHS);
+
     // Create MCDC Decision Region if at top-level (root).
     unsigned NumConds = 0;
     if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
@@ -2057,6 +2159,11 @@ struct CounterCoverageMappingBuilder
     // Create Branch Region around RHS condition.
     createBranchRegion(E->getRHS(), subtractCounters(RHSExecCnt, RHSFalseCnt),
                        RHSFalseCnt, DecisionRHS);
+
+    if (IsRootNode && NumConds > 0 && getRegionBitmap(E) == 0) {
+      RewindDecision(SourceRegionsSince);
+      MCDCState.BitmapMap.erase(E);
+    }
   }
 
   void VisitLambdaExpr(const LambdaExpr *LE) {

clang/lib/CodeGen/MCDCState.h

@@ -26,9 +26,21 @@ using namespace llvm::coverage::mcdc;
 
 /// Per-Function MC/DC state
 struct State {
-  unsigned BitmapBytes = 0;
-  llvm::DenseMap<const Stmt *, unsigned> BitmapMap;
-  llvm::DenseMap<const Stmt *, ConditionID> CondIDMap;
+  unsigned BitmapBits = 0;
+
+  struct Decision {
+    unsigned BitmapIdx;
+    llvm::SmallVector<std::array<int, 2>> Indices;
+  };
+
+  llvm::DenseMap<const Stmt *, Decision> BitmapMap;
+
+  struct Branch {
+    ConditionID ID;
+    const Stmt *DecisionStmt;
+  };
+
+  llvm::DenseMap<const Stmt *, Branch> CondIDMap;
 };
 
 } // namespace clang::CodeGen::MCDC