[llvm] [Coverage] Rework Decision/Expansion/Branch (PR #78969)
NAKAMURA Takumi via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 22 05:09:47 PST 2024
https://github.com/chapuni created https://github.com/llvm/llvm-project/pull/78969
This fixes #77871 and is the counterproposal to #78819
I've fixed almost all issues I found, however, it'd not be straightforward to recall test cases.
>From 3856fd23efc00e0d3c7fec3118a6cc82f4c6fb49 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Sun, 21 Jan 2024 17:56:51 +0900
Subject: [PATCH 1/2] [Coverage] Const-ize `MCDCRecordProcessor` stuff
The life of `MCDCRecordProcessor`'s instance is short.
It may accept `const` objects to process.
On the other hand, the life of `MCDCBranches` is shorter than `Record`.
It may be rewritten with reference, rather than copying.
---
.../ProfileData/Coverage/CoverageMapping.h | 5 +--
.../ProfileData/Coverage/CoverageMapping.cpp | 32 ++++++++++---------
2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 2a857917136a841..33fa17ba811fa73 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -587,8 +587,9 @@ class CounterMappingContext {
/// Return an MCDC record that indicates executed test vectors and condition
/// pairs.
Expected<MCDCRecord>
- evaluateMCDCRegion(CounterMappingRegion Region, BitVector Bitmap,
- ArrayRef<CounterMappingRegion> Branches);
+ evaluateMCDCRegion(const CounterMappingRegion &Region,
+ const BitVector &Bitmap,
+ ArrayRef<const CounterMappingRegion *> Branches);
unsigned getMaxCounterID(const Counter &C) const;
};
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index eece6a2cc717977..3e2746ea759765d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -247,14 +247,14 @@ class MCDCRecordProcessor {
/// Each index of the bitmap corresponds to a possible test vector. An index
/// with a bit value of '1' indicates that the corresponding Test Vector
/// identified by that index was executed.
- BitVector &ExecutedTestVectorBitmap;
+ const BitVector &ExecutedTestVectorBitmap;
/// Decision Region to which the ExecutedTestVectorBitmap applies.
- CounterMappingRegion &Region;
+ const CounterMappingRegion &Region;
/// Array of branch regions corresponding each conditions in the boolean
/// expression.
- ArrayRef<CounterMappingRegion> Branches;
+ ArrayRef<const CounterMappingRegion *> Branches;
/// Total number of conditions in the boolean expression.
unsigned NumConditions;
@@ -276,8 +276,9 @@ class MCDCRecordProcessor {
MCDCRecord::TestVectors ExecVectors;
public:
- MCDCRecordProcessor(BitVector &Bitmap, CounterMappingRegion &Region,
- ArrayRef<CounterMappingRegion> Branches)
+ MCDCRecordProcessor(const BitVector &Bitmap,
+ const CounterMappingRegion &Region,
+ ArrayRef<const CounterMappingRegion *> Branches)
: ExecutedTestVectorBitmap(Bitmap), Region(Region), Branches(Branches),
NumConditions(Region.MCDCParams.NumConditions),
Folded(NumConditions, false), IndependencePairs(NumConditions),
@@ -342,7 +343,7 @@ class MCDCRecordProcessor {
/// Walk the bits in the bitmap. A bit set to '1' indicates that the test
/// vector at the corresponding index was executed during a test run.
- void findExecutedTestVectors(BitVector &ExecutedTestVectorBitmap) {
+ void findExecutedTestVectors(const BitVector &ExecutedTestVectorBitmap) {
for (unsigned Idx = 0; Idx < ExecutedTestVectorBitmap.size(); ++Idx) {
if (ExecutedTestVectorBitmap[Idx] == 0)
continue;
@@ -445,11 +446,11 @@ class MCDCRecordProcessor {
// visualize where the condition is.
// - 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;
- CondLoc[I] = B.startLoc();
- Folded[I++] = (B.Count.isZero() && B.FalseCount.isZero());
+ for (const auto *B : Branches) {
+ Map[B->MCDCParams.ID] = B;
+ PosToID[I] = B->MCDCParams.ID - 1;
+ CondLoc[I] = B->startLoc();
+ Folded[I++] = (B->Count.isZero() && B->FalseCount.isZero());
}
// Initialize a base test vector as 'DontCare'.
@@ -473,8 +474,9 @@ class MCDCRecordProcessor {
};
Expected<MCDCRecord> CounterMappingContext::evaluateMCDCRegion(
- CounterMappingRegion Region, BitVector ExecutedTestVectorBitmap,
- ArrayRef<CounterMappingRegion> Branches) {
+ const CounterMappingRegion &Region,
+ const BitVector &ExecutedTestVectorBitmap,
+ ArrayRef<const CounterMappingRegion *> Branches) {
MCDCRecordProcessor MCDCProcessor(ExecutedTestVectorBitmap, Region, Branches);
return MCDCProcessor.processMCDCRecord();
@@ -638,7 +640,7 @@ Error CoverageMapping::loadFunctionRecord(
unsigned NumConds = 0;
const CounterMappingRegion *MCDCDecision;
- std::vector<CounterMappingRegion> MCDCBranches;
+ std::vector<const CounterMappingRegion *> MCDCBranches;
FunctionRecord Function(OrigFuncName, Record.Filenames);
for (const auto &Region : Record.MappingRegions) {
@@ -666,7 +668,7 @@ Error CoverageMapping::loadFunctionRecord(
// correspond to it in a vector, according to the number of conditions
// recorded for the region (tracked by NumConds).
if (NumConds > 0 && Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
- MCDCBranches.push_back(Region);
+ MCDCBranches.push_back(&Region);
// As we move through all of the MCDCBranchRegions that follow the
// MCDCDecisionRegion, decrement NumConds to make sure we account for
>From 9f97b0b80d5d880fef932933d32528f04e3940dd Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Sun, 21 Jan 2024 20:02:13 +0900
Subject: [PATCH 2/2] [Coverage] Rework Decision/Expansion/Branch (#77871)
---
.../ProfileData/Coverage/CoverageMapping.cpp | 112 +++++++++++++++---
1 file changed, 97 insertions(+), 15 deletions(-)
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 3e2746ea759765d..499359d3b7c8fa1 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -14,6 +14,7 @@
#include "llvm/ProfileData/Coverage/CoverageMapping.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
@@ -582,6 +583,72 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx,
return MaxBitmapID + (SizeInBits / CHAR_BIT);
}
+struct DecisionRow {
+ const CounterMappingRegion *DecisionRegion;
+ LineColPair DecisionStartLoc;
+ LineColPair DecisionEndLoc;
+
+ SmallVector<const CounterMappingRegion *, 6> Branches;
+ DenseSet<CounterMappingRegion::MCDCConditionID> IDs;
+ SmallVector<const CounterMappingRegion *> Expansions;
+
+ DecisionRow(const CounterMappingRegion &Decision)
+ : DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()),
+ DecisionEndLoc(Decision.endLoc()) {}
+
+ bool insert(const CounterMappingRegion &Branch) {
+ auto ID = Branch.MCDCParams.ID;
+ if (ID == 1)
+ Branches.insert(Branches.begin(), &Branch);
+ else
+ Branches.push_back(&Branch);
+ IDs.insert(ID);
+ return (Branches.size() == DecisionRegion->MCDCParams.NumConditions);
+ }
+
+ enum class UpdateResult {
+ NotFound = 0,
+ Updated,
+ Committed,
+ };
+
+ UpdateResult updateBranch(const CounterMappingRegion &Branch) {
+ if (IDs.contains(Branch.MCDCParams.ID))
+ return UpdateResult::NotFound;
+
+ if (Branch.FileID == DecisionRegion->FileID &&
+ Branch.startLoc() >= DecisionStartLoc &&
+ Branch.endLoc() <= DecisionEndLoc)
+ return (insert(Branch) ? UpdateResult::Committed : UpdateResult::Updated);
+
+ for (const auto *R : Expansions) {
+ if (Branch.FileID == R->ExpandedFileID)
+ return (insert(Branch) ? UpdateResult::Committed
+ : UpdateResult::Updated);
+ }
+
+ return UpdateResult::NotFound;
+ }
+
+ bool updateExpansion(const CounterMappingRegion &Expansion) {
+ if (Expansion.FileID == DecisionRegion->FileID &&
+ Expansion.startLoc() >= DecisionStartLoc &&
+ Expansion.endLoc() <= DecisionEndLoc) {
+ Expansions.push_back(&Expansion);
+ return true;
+ }
+
+ for (const auto *R : Expansions) {
+ if (Expansion.FileID == R->ExpandedFileID) {
+ Expansions.push_back(&Expansion);
+ return true;
+ }
+ }
+
+ return false;
+ }
+};
+
Error CoverageMapping::loadFunctionRecord(
const CoverageMappingRecord &Record,
IndexedInstrProfReader &ProfileReader) {
@@ -638,18 +705,11 @@ Error CoverageMapping::loadFunctionRecord(
Record.MappingRegions[0].Count.isZero() && Counts[0] > 0)
return Error::success();
- unsigned NumConds = 0;
- const CounterMappingRegion *MCDCDecision;
- std::vector<const CounterMappingRegion *> MCDCBranches;
-
+ SmallVector<DecisionRow> Decisions;
FunctionRecord Function(OrigFuncName, Record.Filenames);
for (const auto &Region : Record.MappingRegions) {
- // If an MCDCDecisionRegion is seen, track the BranchRegions that follow
- // it according to Region.NumConditions.
if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
- assert(NumConds == 0);
- MCDCDecision = &Region;
- NumConds = Region.MCDCParams.NumConditions;
+ Decisions.emplace_back(Region);
continue;
}
Expected<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
@@ -664,23 +724,39 @@ Error CoverageMapping::loadFunctionRecord(
}
Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);
+ if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
+ for (auto &Decision : reverse(Decisions)) {
+ if (Decision.updateExpansion(Region))
+ break;
+ }
+ continue;
+ }
+
+ if (Region.Kind != CounterMappingRegion::MCDCBranchRegion)
+ continue;
+
// If a MCDCDecisionRegion was seen, store the BranchRegions that
// correspond to it in a vector, according to the number of conditions
// recorded for the region (tracked by NumConds).
- if (NumConds > 0 && Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
- MCDCBranches.push_back(&Region);
+ for (int I = Decisions.size() - 1; I >= 0; --I) {
+ auto &Decision = Decisions[I];
// As we move through all of the MCDCBranchRegions that follow the
// MCDCDecisionRegion, decrement NumConds to make sure we account for
// them all before we calculate the bitmap of executed test vectors.
- if (--NumConds == 0) {
+ switch (Decision.updateBranch(Region)) {
+ case DecisionRow::UpdateResult::NotFound:
+ continue;
+ case DecisionRow::UpdateResult::Updated:
+ goto branch_found;
+ case DecisionRow::UpdateResult::Committed:
// Evaluating the test vector bitmap for the decision region entails
// calculating precisely what bits are pertinent to this region alone.
// This is calculated based on the recorded offset into the global
// profile bitmap; the length is calculated based on the recorded
// number of conditions.
Expected<BitVector> ExecutedTestVectorBitmap =
- Ctx.evaluateBitmap(MCDCDecision);
+ Ctx.evaluateBitmap(Decision.DecisionRegion);
if (auto E = ExecutedTestVectorBitmap.takeError()) {
consumeError(std::move(E));
return Error::success();
@@ -690,7 +766,8 @@ Error CoverageMapping::loadFunctionRecord(
// DecisionRegion, all of the information is now available to process.
// This is where the bulk of the MC/DC progressing takes place.
Expected<MCDCRecord> Record = Ctx.evaluateMCDCRegion(
- *MCDCDecision, *ExecutedTestVectorBitmap, MCDCBranches);
+ *Decision.DecisionRegion, *ExecutedTestVectorBitmap,
+ Decision.Branches);
if (auto E = Record.takeError()) {
consumeError(std::move(E));
return Error::success();
@@ -698,9 +775,14 @@ Error CoverageMapping::loadFunctionRecord(
// Save the MC/DC Record so that it can be visualized later.
Function.pushMCDCRecord(*Record);
- MCDCBranches.clear();
+
+ Decisions.erase(Decisions.begin() + I);
+ goto branch_found;
}
}
+ llvm_unreachable("Branch not found in Decisions");
+
+ branch_found:;
}
// Don't create records for (filenames, function) pairs we've already seen.
More information about the llvm-commits
mailing list