llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-pgo Author: NAKAMURA Takumi (chapuni) <details> <summary>Changes</summary> Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and make sure them not initialized as zero. FIXME: Could we make `CoverageMappingRegion` as a smart tagged union? --- Patch is 25.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81227.diff 6 Files Affected: - (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+39-30) - (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+49-28) - (modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+30-23) - (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+9-8) - (modified) llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp (+18-5) - (modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+4-4) ``````````diff 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/CoverageM... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/81227 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
