diff options
author | Hans Wennborg <hans@chromium.org> | 2024-06-14 10:32:39 +0200 |
---|---|---|
committer | Hans Wennborg <hans@chromium.org> | 2024-06-14 10:47:41 +0200 |
commit | b422fa6b62160f5eeb038d816d05e039182dde56 (patch) | |
tree | 1facf1d549693b6e685af22a7eebbe8b4db9daa4 /clang/lib | |
parent | 880d37038c7bbff53ef02c9d6b01cbbc87875243 (diff) | |
download | llvm-b422fa6b62160f5eeb038d816d05e039182dde56.zip llvm-b422fa6b62160f5eeb038d816d05e039182dde56.tar.gz llvm-b422fa6b62160f5eeb038d816d05e039182dde56.tar.bz2 |
Revert "[MC/DC][Coverage] Loosen the limit of NumConds from 6 (#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 7ead2d8c7e9114b3f23666209a1654939987cb30.
Diffstat (limited to 'clang/lib')
-rw-r--r-- | clang/lib/CodeGen/CodeGenPGO.cpp | 50 | ||||
-rw-r--r-- | clang/lib/CodeGen/CoverageMappingGen.cpp | 77 | ||||
-rw-r--r-- | clang/lib/CodeGen/MCDCState.h | 4 |
3 files changed, 29 insertions, 102 deletions
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp index 59139e3..db8e6f5 100644 --- a/clang/lib/CodeGen/CodeGenPGO.cpp +++ b/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) { diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index ba483d8..6ce2d32 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -195,10 +195,6 @@ public: 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 @@ public: } 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 @@ private: llvm::SmallVector<mcdc::ConditionIDs> DecisionStack; MCDC::State &MCDCState; - const Stmt *DecisionStmt = nullptr; mcdc::ConditionID NextID = 0; bool NotMapped = false; @@ -784,8 +777,7 @@ public: /// 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 @@ public: 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,41 +2122,13 @@ 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, }; @@ -2177,28 +2136,6 @@ struct CounterCoverageMappingBuilder 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) { diff --git a/clang/lib/CodeGen/MCDCState.h b/clang/lib/CodeGen/MCDCState.h index e0dd28f..29b6f0f 100644 --- a/clang/lib/CodeGen/MCDCState.h +++ b/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; |