Revert "[MC/DC][Coverage] Loosen the limit of NumConds from 6 (llvm#82448)"

This broke the lit tests on Mac:
https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/1096/

> By storing possible test vectors instead of combinations of conditions,
> the restriction is dramatically relaxed.
>
> This introduces two options to `cc1`:
>
> * `-fmcdc-max-conditions=32767`
> * `-fmcdc-max-test-vectors=2147483646`
>
> This change makes coverage mapping, profraw, and profdata incompatible
> with Clang-18.
>
> - Bitmap semantics changed. It is incompatible with previous format.
> - `BitmapIdx` in `Decision` points to the end of the bitmap.
> - Bitmap is packed per function.
> - `llvm-cov` can understand `profdata` generated by `llvm-profdata-18`.
>
> RFC:
> https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

This reverts commit 7ead2d8.

Author: zmodem

clang/docs/SourceBasedCodeCoverage.rst

@@ -484,31 +484,10 @@ MC/DC Instrumentation
 ---------------------
 
 When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the
-clang option ``-fcoverage-mcdc``, there are two hard limits.
-
-The maximum number of terms is limited to 32767, which is practical for
-handwritten expressions. To be more restrictive in order to enforce coding rules,
-use ``-Xclang -fmcdc-max-conditions=n``. Expressions with exceeded condition
-counts ``n`` will generate warnings and will be excluded in the MC/DC coverage.
-
-The number of test vectors (the maximum number of possible combinations of
-expressions) is limited to 2,147,483,646. In this case, approximately
-256MiB (==2GiB/8) is used to record test vectors.
-
-To reduce memory usage, users can limit the maximum number of test vectors per
-expression with ``-Xclang -fmcdc-max-test-vectors=m``.
-If the number of test vectors resulting from the analysis of an expression
-exceeds ``m``, a warning will be issued and the expression will be excluded
-from the MC/DC coverage.
-
-The number of test vectors ``m``, for ``n`` terms in an expression, can be
-``m <= 2^n`` in the theoretical worst case, but is usually much smaller.
-In simple cases, such as expressions consisting of a sequence of single
-operators, ``m == n+1``. For example, ``(a && b && c && d && e && f && g)``
-requires 8 test vectors.
-
-Expressions such as ``((a0 && b0) || (a1 && b1) || ...)`` can cause the
-number of test vectors to increase exponentially.
+clang option ``-fcoverage-mcdc``, users are limited to at most **six** leaf-level
+conditions in a boolean expression.  A warning will be generated for boolean
+expressions that contain more than six, and they will not be instrumented for
+MC/DC.
 
 Also, if a boolean expression is embedded in the nest of another boolean
 expression but separated by a non-logical operator, this is also not supported.

clang/include/clang/Basic/CodeGenOptions.def

@@ -223,8 +223,6 @@ CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to
 CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
                                        ///< regions.
 CODEGENOPT(MCDCCoverage , 1, 0) ///< Enable MC/DC code coverage criteria.
-VALUE_CODEGENOPT(MCDCMaxConds, 16, 32767) ///< MC/DC Maximum conditions.
-VALUE_CODEGENOPT(MCDCMaxTVs, 32, 0x7FFFFFFE) ///< MC/DC Maximum test vectors.
 
   /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)

clang/include/clang/Driver/Options.td

@@ -1790,14 +1790,6 @@ defm mcdc_coverage : BoolFOption<"coverage-mcdc",
           "Enable MC/DC criteria when generating code coverage">,
   NegFlag<SetFalse, [], [ClangOption], "Disable MC/DC coverage criteria">,
           BothFlags<[], [ClangOption, CLOption]>>;
-def fmcdc_max_conditions_EQ : Joined<["-"], "fmcdc-max-conditions=">,
-  Group<f_Group>, Visibility<[CC1Option]>,
-  HelpText<"Maximum number of conditions in MC/DC coverage">,
-  MarshallingInfoInt<CodeGenOpts<"MCDCMaxConds">, "32767">;
-def fmcdc_max_test_vectors_EQ : Joined<["-"], "fmcdc-max-test-vectors=">,
-  Group<f_Group>, Visibility<[CC1Option]>,
-  HelpText<"Maximum number of test vectors in MC/DC coverage">,
-  MarshallingInfoInt<CodeGenOpts<"MCDCMaxTVs">, "0x7FFFFFFE">;
 def fprofile_generate : Flag<["-"], "fprofile-generate">,
     Group<f_Group>, Visibility<[ClangOption, CLOption]>,
     HelpText<"Generate instrumented code to collect execution counts into default.profraw (overridden by LLVM_PROFILE_FILE env var)">;

clang/lib/CodeGen/CodeGenPGO.cpp

@@ -167,6 +167,8 @@ 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.
@@ -181,7 +183,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
                     MCDC::State &MCDCState, unsigned MCDCMaxCond,
                     DiagnosticsEngine &Diag)
       : NextCounter(0), Hash(HashVersion), CounterMap(CounterMap),
-        MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
+        NextMCDCBitmapIdx(0), MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
         ProfileVersion(ProfileVersion), Diag(Diag) {}
 
   // Blocks and lambdas are handled as separate functions, so we need not
@@ -312,8 +314,11 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
             return true;
           }
 
-          // Otherwise, allocate the Decision.
-          MCDCState.DecisionByStmt[BinOp].BitmapIdx = 0;
+          // Otherwise, allocate the number of bytes required for the bitmap
+          // based on the number of conditions. Must be at least 1-byte long.
+          MCDCState.DecisionByStmt[BinOp].BitmapIdx = NextMCDCBitmapIdx;
+          unsigned SizeInBits = std::max<unsigned>(1L << NumCond, CHAR_BIT);
+          NextMCDCBitmapIdx += SizeInBits / CHAR_BIT;
         }
         return true;
       }
@@ -1078,9 +1083,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 ? CGM.getCodeGenOpts().MCDCMaxConds
-                                         : 0);
+  unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
 
   RegionCounterMap.reset(new llvm::DenseMap<const Stmt *, unsigned>);
   RegionMCDCState.reset(new MCDC::State);
@@ -1096,6 +1099,7 @@ 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();
 }
 
@@ -1228,7 +1232,7 @@ void CodeGenPGO::emitMCDCParameters(CGBuilderTy &Builder) {
   // anything.
   llvm::Value *Args[3] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
                           Builder.getInt64(FunctionHash),
-                          Builder.getInt32(RegionMCDCState->BitmapBits)};
+                          Builder.getInt32(RegionMCDCState->BitmapBytes)};
   Builder.CreateCall(
       CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_parameters), Args);
 }
@@ -1246,11 +1250,6 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
   if (DecisionStateIter == RegionMCDCState->DecisionByStmt.end())
     return;
 
-  // Don't create tvbitmap_update if the record is allocated but excluded.
-  // Or `bitmap |= (1 << 0)` would be wrongly executed to the next bitmap.
-  if (DecisionStateIter->second.Indices.size() == 0)
-    return;
-
   // Extract the offset of the global bitmap associated with this expression.
   unsigned MCDCTestVectorBitmapOffset = DecisionStateIter->second.BitmapIdx;
   auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
@@ -1262,7 +1261,7 @@ 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(0), // Unused
+                          Builder.getInt32(RegionMCDCState->BitmapBytes),
                           Builder.getInt32(MCDCTestVectorBitmapOffset),
                           MCDCCondBitmapAddr.emitRawPointer(CGF)};
   Builder.CreateCall(
@@ -1306,22 +1305,19 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
   // Extract the ID of the condition we are setting in the bitmap.
   const auto &Branch = BranchStateIter->second;
   assert(Branch.ID >= 0 && "Condition has no ID!");
-  assert(Branch.DecisionStmt);
-
-  // Cancel the emission if the Decision is erased after the allocation.
-  const auto DecisionIter =
-      RegionMCDCState->DecisionByStmt.find(Branch.DecisionStmt);
-  if (DecisionIter == RegionMCDCState->DecisionByStmt.end())
-    return;
 
-  const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
+  auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
 
-  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);
+  // 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(Branch.ID),
+                          MCDCCondBitmapAddr.emitRawPointer(CGF), Val};
+  Builder.CreateCall(
+      CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_condbitmap_update),
+      Args);
 }
 
 void CodeGenPGO::setValueProfilingFlag(llvm::Module &M) {

clang/lib/CodeGen/CoverageMappingGen.cpp

@@ -195,10 +195,6 @@ class SourceMappingRegion {
     return std::holds_alternative<mcdc::BranchParameters>(MCDCParams);
   }
 
-  const auto &getMCDCBranchParams() const {
-    return mcdc::getParams<const mcdc::BranchParameters>(MCDCParams);
-  }
-
   bool isMCDCDecision() const {
     return std::holds_alternative<mcdc::DecisionParameters>(MCDCParams);
   }
@@ -208,8 +204,6 @@ 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.
@@ -754,7 +748,6 @@ struct MCDCCoverageBuilder {
 
   llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
   MCDC::State &MCDCState;
-  const Stmt *DecisionStmt = nullptr;
   mcdc::ConditionID NextID = 0;
   bool NotMapped = false;
 
@@ -784,8 +777,7 @@ struct MCDCCoverageBuilder {
 
   /// Set the given condition's ID.
   void setCondID(const Expr *Cond, mcdc::ConditionID ID) {
-    MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {ID,
-                                                                DecisionStmt};
+    MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)].ID = ID;
   }
 
   /// Return the ID of a given condition.
@@ -816,11 +808,6 @@ struct MCDCCoverageBuilder {
     if (NotMapped)
       return;
 
-    if (NextID == 0) {
-      DecisionStmt = E;
-      assert(MCDCState.DecisionByStmt.contains(E));
-    }
-
     const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();
 
     // If the operator itself has an assigned ID, this means it represents a
@@ -2135,70 +2122,20 @@ struct CounterCoverageMappingBuilder
                          subtractCounters(ParentCount, TrueCount));
   }
 
-  void createOrCancelDecision(const BinaryOperator *E, unsigned Since) {
+  void createDecision(const BinaryOperator *E) {
     unsigned NumConds = MCDCBuilder.getTotalConditionsAndReset(E);
     if (NumConds == 0)
       return;
 
-    // Extract [ID, Conds] to construct the graph.
-    llvm::SmallVector<mcdc::ConditionIDs> CondIDs(NumConds);
-    for (const auto &SR : ArrayRef(SourceRegions).slice(Since)) {
-      if (SR.isMCDCBranch()) {
-        auto [ID, Conds] = SR.getMCDCBranchParams();
-        CondIDs[ID] = Conds;
-      }
-    }
-
-    // Construct the graph and calculate `Indices`.
-    mcdc::TVIdxBuilder Builder(CondIDs);
-    unsigned NumTVs = Builder.NumTestVectors;
-    unsigned MaxTVs = CVM.getCodeGenModule().getCodeGenOpts().MCDCMaxTVs;
-    assert(MaxTVs < mcdc::TVIdxBuilder::HardMaxTVs);
-
-    if (NumTVs > MaxTVs) {
-      // NumTVs exceeds MaxTVs -- warn and cancel the Decision.
-      cancelDecision(E, Since, NumTVs, MaxTVs);
-      return;
-    }
-
-    // Update the state for CodeGenPGO
-    assert(MCDCState.DecisionByStmt.contains(E));
-    MCDCState.DecisionByStmt[E] = {
-        MCDCState.BitmapBits, // Top
-        std::move(Builder.Indices),
-    };
-
     auto DecisionParams = mcdc::DecisionParameters{
-        MCDCState.BitmapBits += NumTVs, // Tail
+        MCDCState.DecisionByStmt[E].BitmapIdx,
         NumConds,
     };
 
     // Create MCDC Decision Region.
     createDecisionRegion(E, DecisionParams);
   }
 
-  // Warn and cancel the Decision.
-  void cancelDecision(const BinaryOperator *E, unsigned Since, int NumTVs,
-                      int MaxTVs) {
-    auto &Diag = CVM.getCodeGenModule().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;
-
-    // Restore MCDCBranch to Branch.
-    for (auto &SR : MutableArrayRef(SourceRegions).slice(Since)) {
-      assert(!SR.isMCDCDecision() && "Decision shouldn't be seen here");
-      if (SR.isMCDCBranch())
-        SR.resetMCDCParams();
-    }
-
-    // Tell CodeGenPGO not to instrument.
-    MCDCState.DecisionByStmt.erase(E);
-  }
-
   /// Check if E belongs to system headers.
   bool isExprInSystemHeader(const BinaryOperator *E) const {
     return (!SystemHeadersCoverage &&
@@ -2215,8 +2152,6 @@ struct CounterCoverageMappingBuilder
 
     bool IsRootNode = MCDCBuilder.isIdle();
 
-    unsigned SourceRegionsSince = SourceRegions.size();
-
     // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
@@ -2255,7 +2190,7 @@ struct CounterCoverageMappingBuilder
 
     // Create MCDC Decision Region if at top-level (root).
     if (IsRootNode)
-      createOrCancelDecision(E, SourceRegionsSince);
+      createDecision(E);
   }
 
   // Determine whether the right side of OR operation need to be visited.
@@ -2276,8 +2211,6 @@ struct CounterCoverageMappingBuilder
 
     bool IsRootNode = MCDCBuilder.isIdle();
 
-    unsigned SourceRegionsSince = SourceRegions.size();
-
     // Keep track of Binary Operator and assign MCDC condition IDs.
     MCDCBuilder.pushAndAssignIDs(E);
 
@@ -2320,7 +2253,7 @@ struct CounterCoverageMappingBuilder
 
     // Create MCDC Decision Region if at top-level (root).
     if (IsRootNode)
-      createOrCancelDecision(E, SourceRegionsSince);
+      createDecision(E);
   }
 
   void VisitLambdaExpr(const LambdaExpr *LE) {

clang/lib/CodeGen/MCDCState.h

@@ -27,18 +27,16 @@ using namespace llvm::coverage::mcdc;
 
 /// Per-Function MC/DC state
 struct State {
-  unsigned BitmapBits = 0;
+  unsigned BitmapBytes = 0;
 
   struct Decision {
     unsigned BitmapIdx;
-    llvm::SmallVector<std::array<int, 2>> Indices;
   };
 
   llvm::DenseMap<const Stmt *, Decision> DecisionByStmt;
 
   struct Branch {
     ConditionID ID;
-    const Stmt *DecisionStmt;
   };
 
   llvm::DenseMap<const Stmt *, Branch> BranchByStmt;