[llvm] [Coverage] Rework Decision/Expansion/Branch (PR #78969)
NAKAMURA Takumi via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 23 04:47:26 PST 2024
https://github.com/chapuni updated https://github.com/llvm/llvm-project/pull/78969
>From 5d9de08db9025b67aecfcce65cee0d866759ce06 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 1/4] [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 9c95cd5d76d2151..d8e1af84e898aa8 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.
>From 1564424ec4018fc2a26a3263cbabba2a4a6cbec7 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Tue, 23 Jan 2024 14:48:36 +0900
Subject: [PATCH 2/4] Confirm that all Decisions have not been resolved.
---
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index d8e1af84e898aa8..b8bff6df4ab0e9d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -785,6 +785,8 @@ Error CoverageMapping::loadFunctionRecord(
branch_found:;
}
+ assert(Decisions.empty() && "All Decisions have not been resolved");
+
// Don't create records for (filenames, function) pairs we've already seen.
auto FilenamesHash = hash_combine_range(Record.Filenames.begin(),
Record.Filenames.end());
>From f74dec7611aa997bb7973589aef06ef17d0dad11 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Tue, 23 Jan 2024 14:35:09 +0900
Subject: [PATCH 3/4] DecisionRow: Rework to reflect reviews
---
.../ProfileData/Coverage/CoverageMapping.cpp | 63 +++++++++----------
1 file changed, 28 insertions(+), 35 deletions(-)
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index b8bff6df4ab0e9d..5aa25c8b8b11b48 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -596,56 +596,49 @@ struct DecisionRow {
: 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);
+ bool inDecisionRegion(const CounterMappingRegion &R) {
+ return (R.FileID == DecisionRegion->FileID &&
+ R.startLoc() >= DecisionStartLoc && R.endLoc() <= DecisionEndLoc);
+ }
+
+ bool inExpansions(const CounterMappingRegion &R) {
+ return any_of(Expansions, [&](const auto &Expansion) {
+ return (Expansion->ExpandedFileID == R.FileID);
+ });
}
enum class UpdateResult {
NotFound = 0,
Updated,
- Committed,
+ Completed,
};
UpdateResult updateBranch(const CounterMappingRegion &Branch) {
- if (IDs.contains(Branch.MCDCParams.ID))
- return UpdateResult::NotFound;
+ auto ID = Branch.MCDCParams.ID;
- if (Branch.FileID == DecisionRegion->FileID &&
- Branch.startLoc() >= DecisionStartLoc &&
- Branch.endLoc() <= DecisionEndLoc)
- return (insert(Branch) ? UpdateResult::Committed : UpdateResult::Updated);
+ if (!IDs.contains(ID) &&
+ (inDecisionRegion(Branch) || inExpansions(Branch))) {
+ assert(Branches.size() < DecisionRegion->MCDCParams.NumConditions);
- for (const auto *R : Expansions) {
- if (Branch.FileID == R->ExpandedFileID)
- return (insert(Branch) ? UpdateResult::Committed
- : UpdateResult::Updated);
- }
+ if (ID == 1)
+ Branches.insert(Branches.begin(), &Branch);
+ else
+ Branches.push_back(&Branch);
- return UpdateResult::NotFound;
+ IDs.insert(ID);
+ return (Branches.size() == DecisionRegion->MCDCParams.NumConditions
+ ? UpdateResult::Completed
+ : UpdateResult::Updated);
+ } else
+ return UpdateResult::NotFound;
}
bool updateExpansion(const CounterMappingRegion &Expansion) {
- if (Expansion.FileID == DecisionRegion->FileID &&
- Expansion.startLoc() >= DecisionStartLoc &&
- Expansion.endLoc() <= DecisionEndLoc) {
+ if (inDecisionRegion(Expansion) || inExpansions(Expansion)) {
Expansions.push_back(&Expansion);
return true;
- }
-
- for (const auto *R : Expansions) {
- if (Expansion.FileID == R->ExpandedFileID) {
- Expansions.push_back(&Expansion);
- return true;
- }
- }
-
- return false;
+ } else
+ return false;
}
};
@@ -749,7 +742,7 @@ Error CoverageMapping::loadFunctionRecord(
continue;
case DecisionRow::UpdateResult::Updated:
goto branch_found;
- case DecisionRow::UpdateResult::Committed:
+ case DecisionRow::UpdateResult::Completed:
// 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
>From 425185ce38676d6b5ccd0e18c70038b28771291b Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Tue, 23 Jan 2024 21:46:06 +0900
Subject: [PATCH 4/4] Add comments to the upper half.
---
.../ProfileData/Coverage/CoverageMapping.cpp | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 5aa25c8b8b11b48..61984adae3f2087 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -583,24 +583,37 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx,
return MaxBitmapID + (SizeInBits / CHAR_BIT);
}
+/// This holds the DecisionRegion and MCDCBranch(es) under it.
+/// Also traverses Expansion(s).
struct DecisionRow {
+ /// The subject
const CounterMappingRegion *DecisionRegion;
+
+ /// They are reflected from `DecisionRegion` for convenience.
LineColPair DecisionStartLoc;
LineColPair DecisionEndLoc;
+ /// This is passed to `MCDCRecordProcessor`, so this should be compatible to
+ /// `ArrayRef<const CounterMappingRegion *>`.
SmallVector<const CounterMappingRegion *, 6> Branches;
+
+ /// Each `ID` in `Branches` should be unique.
DenseSet<CounterMappingRegion::MCDCConditionID> IDs;
+
+ /// Relevant `Expansion`(s) should be caught to find expanded Branches.
SmallVector<const CounterMappingRegion *> Expansions;
DecisionRow(const CounterMappingRegion &Decision)
: DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()),
DecisionEndLoc(Decision.endLoc()) {}
+ /// Determine whether `R` is included in `DecisionRegion`.
bool inDecisionRegion(const CounterMappingRegion &R) {
return (R.FileID == DecisionRegion->FileID &&
R.startLoc() >= DecisionStartLoc && R.endLoc() <= DecisionEndLoc);
}
+ /// Determin whether `R` is pointed by any of Expansions.
bool inExpansions(const CounterMappingRegion &R) {
return any_of(Expansions, [&](const auto &Expansion) {
return (Expansion->ExpandedFileID == R.FileID);
@@ -613,19 +626,26 @@ struct DecisionRow {
Completed,
};
+ /// Add `Branch` into the Decision
UpdateResult updateBranch(const CounterMappingRegion &Branch) {
auto ID = Branch.MCDCParams.ID;
+ assert(ID > 0 && "MCDCBranch.ID should begin with 1");
if (!IDs.contains(ID) &&
(inDecisionRegion(Branch) || inExpansions(Branch))) {
assert(Branches.size() < DecisionRegion->MCDCParams.NumConditions);
+ // Put `ID=1` in front of `Branches` for convenience
+ // even if `Branches` is not topological.
if (ID == 1)
Branches.insert(Branches.begin(), &Branch);
else
Branches.push_back(&Branch);
+ // Mark `ID` as `assigned`.
IDs.insert(ID);
+
+ // `Completed` when `Branches` is full
return (Branches.size() == DecisionRegion->MCDCParams.NumConditions
? UpdateResult::Completed
: UpdateResult::Updated);
@@ -633,6 +653,8 @@ struct DecisionRow {
return UpdateResult::NotFound;
}
+ /// Record `Expansion` if it is dominated to the Decision.
+ /// Each `Expansion` may nest.
bool updateExpansion(const CounterMappingRegion &Expansion) {
if (inDecisionRegion(Expansion) || inExpansions(Expansion)) {
Expansions.push_back(&Expansion);
@@ -702,6 +724,7 @@ Error CoverageMapping::loadFunctionRecord(
FunctionRecord Function(OrigFuncName, Record.Filenames);
for (const auto &Region : Record.MappingRegions) {
if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
+ // Start recording `Region` as the `Decision`
Decisions.emplace_back(Region);
continue;
}
More information about the llvm-commits
mailing list