[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 8 21:19:41 PST 2024
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
More information about the cfe-commits
mailing list