[MC/DC] Refactor: Introduce ConditionIDs as std::array<2>
#81221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Its 0th element corresponds to `FalseID` and 1st to `TrueID`. CoverageMappingGen.cpp: `DecisionIDPair` is replaced with `ConditionIDs`
llvmbot
added
clang
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-clang Author: NAKAMURA Takumi (chapuni) ChangesIts 0th element corresponds to CoverageMappingGen.cpp: Full diff: https://github.com/llvm/llvm-project/pull/81221.diff 6 Files Affected:
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..54559af47e63dc 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -95,7 +95,6 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) {
}
namespace {
-using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
using MCDCParameters = CounterMappingRegion::MCDCParameters;
/// A region of source code that can be mapped to a counter.
@@ -586,11 +585,6 @@ struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {
/// creation.
struct MCDCCoverageBuilder {
- struct DecisionIDPair {
- MCDCConditionID TrueID = 0;
- MCDCConditionID FalseID = 0;
- };
-
/// The AST walk recursively visits nested logical-AND or logical-OR binary
/// operator nodes and then visits their LHS and RHS children nodes. As this
/// happens, the algorithm will assign IDs to each operator's LHS and RHS side
@@ -681,14 +675,14 @@ struct MCDCCoverageBuilder {
private:
CodeGenModule &CGM;
- llvm::SmallVector<DecisionIDPair> DecisionStack;
+ llvm::SmallVector<MCDCConditionIDs> DecisionStack;
llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDs;
llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap;
MCDCConditionID NextID = 1;
bool NotMapped = false;
/// Represent a sentinel value of [0,0] for the bottom of DecisionStack.
- static constexpr DecisionIDPair DecisionStackSentinel{0, 0};
+ static constexpr MCDCConditionIDs DecisionStackSentinel{0, 0};
/// Is this a logical-AND operation?
bool isLAnd(const BinaryOperator *E) const {
@@ -727,7 +721,7 @@ struct MCDCCoverageBuilder {
}
/// Return the LHS Decision ([0,0] if not set).
- const DecisionIDPair &back() const { return DecisionStack.back(); }
+ const MCDCConditionIDs &back() const { return DecisionStack.back(); }
/// Push the binary operator statement to track the nest level and assign IDs
/// to the operator's LHS and RHS. The RHS may be a larger subtree that is
@@ -744,7 +738,7 @@ struct MCDCCoverageBuilder {
if (NotMapped)
return;
- const DecisionIDPair &ParentDecision = DecisionStack.back();
+ const MCDCConditionIDs &ParentDecision = DecisionStack.back();
// If the operator itself has an assigned ID, this means it represents a
// larger subtree. In this case, assign that ID to its LHS node. Its RHS
@@ -760,18 +754,18 @@ struct MCDCCoverageBuilder {
// Push the LHS decision IDs onto the DecisionStack.
if (isLAnd(E))
- DecisionStack.push_back({RHSid, ParentDecision.FalseID});
+ DecisionStack.push_back({ParentDecision[0], RHSid});
else
- DecisionStack.push_back({ParentDecision.TrueID, RHSid});
+ DecisionStack.push_back({RHSid, ParentDecision[1]});
}
/// Pop and return the LHS Decision ([0,0] if not set).
- DecisionIDPair pop() {
+ MCDCConditionIDs pop() {
if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
return DecisionStack.front();
assert(DecisionStack.size() > 1);
- DecisionIDPair D = DecisionStack.back();
+ MCDCConditionIDs D = DecisionStack.back();
DecisionStack.pop_back();
return D;
}
@@ -865,8 +859,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) {
+ MCDCConditionID ID = 0, MCDCConditionIDs Conds = {}) {
if (StartLoc && !FalseCount) {
MostRecentLocation = *StartLoc;
@@ -885,8 +878,7 @@ struct CounterCoverageMappingBuilder
StartLoc = std::nullopt;
if (EndLoc && EndLoc->isInvalid())
EndLoc = std::nullopt;
- RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
+ RegionStack.emplace_back(Count, FalseCount, MCDCParameters{0, 0, ID, Conds},
StartLoc, EndLoc);
return RegionStack.size() - 1;
@@ -1024,15 +1016,12 @@ struct CounterCoverageMappingBuilder
return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
}
- using MCDCDecisionIDPair = MCDCCoverageBuilder::DecisionIDPair;
-
/// Create a Branch Region around an instrumentable condition for coverage
/// and add it to the function's SourceRegions. A branch region tracks a
/// "True" counter and a "False" counter for boolean expressions that
/// result in the generation of a branch.
- void
- createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
- const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {
+ void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
+ const MCDCConditionIDs &Conds = {}) {
// Check for NULL conditions.
if (!C)
return;
@@ -1043,8 +1032,6 @@ struct CounterCoverageMappingBuilder
// code other than the Condition.
if (CodeGenFunction::isInstrumentedCondition(C)) {
MCDCConditionID ID = MCDCBuilder.getCondID(C);
- MCDCConditionID TrueID = IDPair.TrueID;
- MCDCConditionID FalseID = 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 +1041,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(), ID, Conds));
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, ID, Conds));
}
}
@@ -1152,8 +1139,7 @@ struct CounterCoverageMappingBuilder
SourceRegions.emplace_back(
I.getCounter(), I.getFalseCounter(),
MCDCParameters{0, 0, I.getMCDCParams().ID,
- I.getMCDCParams().TrueID,
- I.getMCDCParams().FalseID},
+ I.getMCDCParams().Conds},
Loc, getEndOfFileOrMacro(Loc), I.isBranch());
else
SourceRegions.emplace_back(I.getCounter(), Loc,
@@ -2134,8 +2120,8 @@ 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 << "] ";
+ OS << " [" << R.MCDCParams.ID << "," << R.MCDCParams.Conds[1];
+ OS << "," << R.MCDCParams.Conds[0] << "] ";
}
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..d07baa8e71a29f 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -30,6 +30,7 @@
#include "llvm/Support/Endian.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_ostream.h"
+#include <array>
#include <cassert>
#include <cstdint>
#include <iterator>
@@ -37,7 +38,6 @@
#include <sstream>
#include <string>
#include <system_error>
-#include <tuple>
#include <utility>
#include <vector>
@@ -217,6 +217,9 @@ class CounterExpressionBuilder {
using LineColPair = std::pair<unsigned, unsigned>;
+using MCDCConditionID = unsigned int;
+using MCDCConditionIDs = std::array<MCDCConditionID, 2>;
+
/// A Counter mapping region associates a source range with a specific counter.
struct CounterMappingRegion {
enum RegionKind {
@@ -249,7 +252,6 @@ struct CounterMappingRegion {
MCDCBranchRegion
};
- using MCDCConditionID = unsigned int;
struct MCDCParameters {
/// Byte Index of Bitmap Coverage Object for a Decision Region.
unsigned BitmapIdx = 0;
@@ -259,7 +261,8 @@ struct CounterMappingRegion {
/// 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 = 0;
+ MCDCConditionIDs Conds;
};
/// Primary Counter that is also used for Branch Regions (TrueCount).
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index eb0996e33b70dc..66152685af831b 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -245,7 +245,7 @@ class MCDCRecordProcessor {
unsigned BitmapIdx;
/// Mapping of a condition ID to its corresponding branch region.
- llvm::DenseMap<unsigned, const CounterMappingRegion *> Map;
+ llvm::DenseMap<unsigned, MCDCConditionIDs> CondsMap;
/// Vector used to track whether a condition is constant folded.
MCDCRecord::BoolVector Folded;
@@ -285,20 +285,17 @@ class MCDCRecordProcessor {
// the truth table.
void buildTestVector(MCDCRecord::TestVector &TV, unsigned ID,
unsigned Index) {
- const CounterMappingRegion *Branch = Map[ID];
-
- TV[ID - 1] = MCDCRecord::MCDC_False;
- if (Branch->MCDCParams.FalseID > 0)
- buildTestVector(TV, Branch->MCDCParams.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);
- else
- recordTestVector(TV, Index, MCDCRecord::MCDC_True);
+ const auto &Conds = CondsMap[ID];
+
+ for (unsigned I = 0; I < 2; ++I) {
+ auto MCDCCond = (I ? MCDCRecord::MCDC_True : MCDCRecord::MCDC_False);
+ Index |= I << (ID - 1);
+ TV[ID - 1] = MCDCCond;
+ if (Conds[I] > 0)
+ buildTestVector(TV, Conds[I], Index);
+ else
+ recordTestVector(TV, Index, MCDCCond);
+ }
// Reset back to DontCare.
TV[ID - 1] = MCDCRecord::MCDC_DontCare;
@@ -371,7 +368,7 @@ 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;
+ CondsMap[B->MCDCParams.ID] = B->MCDCParams.Conds;
PosToID[I] = B->MCDCParams.ID - 1;
CondLoc[I] = B->startLoc();
Folded[I++] = (B->Count.isZero() && B->FalseCount.isZero());
@@ -524,7 +521,7 @@ class MCDCDecisionRecorder {
/// IDs that are stored in MCDCBranches
/// Complete when all IDs (1 to NumConditions) are met.
- DenseSet<CounterMappingRegion::MCDCConditionID> ConditionIDs;
+ DenseSet<MCDCConditionID> ConditionIDs;
/// Set of IDs of Expansion(s) that are relevant to DecisionRegion
/// and its children (via expansions).
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index ac8e6b56379f21..351c16397da8f0 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -372,9 +372,10 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
auto CMR = CounterMappingRegion(
C, C2,
CounterMappingRegion::MCDCParameters{
- static_cast<unsigned>(BIDX), static_cast<unsigned>(NC),
- static_cast<unsigned>(ID), static_cast<unsigned>(TID),
- static_cast<unsigned>(FID)},
+ static_cast<unsigned>(BIDX),
+ static_cast<unsigned>(NC),
+ static_cast<unsigned>(ID),
+ {static_cast<unsigned>(FID), static_cast<unsigned>(TID)}},
InferredFileID, ExpandedFileID, LineStart, ColumnStart,
LineStart + NumLines, ColumnEnd, Kind);
if (CMR.startLoc() > CMR.endLoc())
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 27727f216b0513..5a5d546de8bbb7 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -252,8 +252,8 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
writeCounter(MinExpressions, Count, OS);
writeCounter(MinExpressions, FalseCount, OS);
encodeULEB128(unsigned(I->MCDCParams.ID), OS);
- encodeULEB128(unsigned(I->MCDCParams.TrueID), OS);
- encodeULEB128(unsigned(I->MCDCParams.FalseID), OS);
+ encodeULEB128(unsigned(I->MCDCParams.Conds[1]), OS);
+ encodeULEB128(unsigned(I->MCDCParams.Conds[0]), OS);
break;
case CounterMappingRegion::MCDCDecisionRegion:
encodeULEB128(unsigned(I->Kind)
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index 2849781a9dc43b..c26c096fdf9f36 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -197,18 +197,18 @@ struct CoverageMappingTest : ::testing::TestWithParam<std::tuple<bool, bool>> {
auto &Regions = InputFunctions.back().Regions;
unsigned FileID = getFileIndexForFunction(File);
Regions.push_back(CounterMappingRegion::makeDecisionRegion(
- CounterMappingRegion::MCDCParameters{Mask, NC}, FileID, LS, CS, LE,
- CE));
+ CounterMappingRegion::MCDCParameters{Mask, NC, 0, {}}, FileID, LS, CS,
+ LE, CE));
}
- void addMCDCBranchCMR(Counter C1, Counter C2, unsigned ID, unsigned TrueID,
- unsigned FalseID, StringRef File, unsigned LS,
+ void addMCDCBranchCMR(Counter C1, Counter C2, unsigned ID,
+ MCDCConditionIDs Conds, StringRef File, unsigned LS,
unsigned CS, unsigned LE, unsigned CE) {
auto &Regions = InputFunctions.back().Regions;
unsigned FileID = getFileIndexForFunction(File);
Regions.push_back(CounterMappingRegion::makeBranchRegion(
- C1, C2, CounterMappingRegion::MCDCParameters{0, 0, ID, TrueID, FalseID},
- FileID, LS, CS, LE, CE));
+ C1, C2, CounterMappingRegion::MCDCParameters{0, 0, ID, Conds}, FileID,
+ LS, CS, LE, CE));
}
void addExpansionCMR(StringRef File, StringRef ExpandedFile, unsigned LS,
@@ -874,9 +874,9 @@ TEST_P(CoverageMappingTest, non_code_region_bitmask) {
addCMR(Counter::getCounter(3), "file", 1, 1, 5, 5);
addMCDCDecisionCMR(0, 2, "file", 7, 1, 7, 6);
- addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, 2, 0,
+ addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, {0, 2},
"file", 7, 2, 7, 3);
- addMCDCBranchCMR(Counter::getCounter(2), Counter::getCounter(3), 2, 0, 0,
+ addMCDCBranchCMR(Counter::getCounter(2), Counter::getCounter(3), 2, {0, 0},
"file", 7, 4, 7, 5);
EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
@@ -902,11 +902,11 @@ TEST_P(CoverageMappingTest, decision_before_expansion) {
addExpansionCMR("foo", "B", 4, 19, 4, 20);
addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
- addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, 2, 0, "A",
- 1, 14, 1, 17);
+ addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, {0, 2},
+ "A", 1, 14, 1, 17);
addCMR(Counter::getCounter(1), "B", 1, 14, 1, 17);
- addMCDCBranchCMR(Counter::getCounter(1), Counter::getCounter(2), 2, 0, 0, "B",
- 1, 14, 1, 17);
+ addMCDCBranchCMR(Counter::getCounter(1), Counter::getCounter(2), 2, {0, 0},
+ "B", 1, 14, 1, 17);
// InputFunctionCoverageData::Regions is rewritten after the write.
auto InputRegions = InputFunctions.back().Regions;
|
evodius96
reviewed
Feb 9, 2024
| @@ -217,6 +217,9 @@ class CounterExpressionBuilder { | |||
|
|
|||
| using LineColPair = std::pair<unsigned, unsigned>; | |||
|
|
|||
| using MCDCConditionID = unsigned int; | |||
| using MCDCConditionIDs = std::array<MCDCConditionID, 2>; | |||
There was a problem hiding this comment.
is std::array<> better than std::pair<> when it only has two elements?
There was a problem hiding this comment.
first and second didn't make sense to me.
|
@evodius96 As an experiment, I've made subscripts as I can rewind it. |
evodius96
approved these changes
Feb 13, 2024
chapuni
added a commit
that referenced
this pull request
Feb 15, 2024
Also, Let `NumConditions` `uint16_t`. It is smarter to handle the ID as signed. Narrowing to `int16_t` will reduce costs of handling byvalue. (See also #81221 and #81227) External behavior doesn't change. They below handle values as internal values plus 1. * `-dump-coverage-mapping` * `CoverageMappingReader.cpp` * `CoverageMappingWriter.cpp`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Its 0th element corresponds to
FalseIDand 1st toTrueID.CoverageMappingGen.cpp:
DecisionIDPairis replaced withConditionIDs