Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Coverage][Single] Round Counters to boolean after evaluation #110972

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[Coverage][Single] Round Counters to boolean after evaluation #110972

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
178b57c
[Coverage] Move SingleByteCoverage out of CountedRegion
chapuni Oct 2, 2024
aacb50d
[Coverage] Make SingleByteCoverage work consistent to merging
chapuni Oct 2, 2024
b9bbc7c
Rework. (Also reverts "[Coverage] Move SingleByteCoverage out of Cou…
chapuni Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler-rt/test/profile/instrprof-block-coverage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,4 @@ int main(int argc, char *argv[]) {

// CHECK-ERROR-NOT: warning: {{.*}}: Found inconsistent block coverage

// COUNTS: Maximum function count: 4
// COUNTS: Maximum function count: 1
2 changes: 1 addition & 1 deletion compiler-rt/test/profile/instrprof-entry-coverage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ int main(int argc, char *argv[]) {
// CHECK-DAG: foo
// CHECK-DAG: bar

// COUNTS: Maximum function count: 2
// COUNTS: Maximum function count: 1
27 changes: 12 additions & 15 deletions llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,19 +359,15 @@ struct CountedRegion : public CounterMappingRegion {
uint64_t ExecutionCount;
uint64_t FalseExecutionCount;
bool Folded;
bool HasSingleByteCoverage;

CountedRegion(const CounterMappingRegion &R, uint64_t ExecutionCount,
bool HasSingleByteCoverage)
CountedRegion(const CounterMappingRegion &R, uint64_t ExecutionCount)
: CounterMappingRegion(R), ExecutionCount(ExecutionCount),
FalseExecutionCount(0), Folded(false),
HasSingleByteCoverage(HasSingleByteCoverage) {}
FalseExecutionCount(0), Folded(false) {}

CountedRegion(const CounterMappingRegion &R, uint64_t ExecutionCount,
uint64_t FalseExecutionCount, bool HasSingleByteCoverage)
uint64_t FalseExecutionCount)
: CounterMappingRegion(R), ExecutionCount(ExecutionCount),
FalseExecutionCount(FalseExecutionCount), Folded(false),
HasSingleByteCoverage(HasSingleByteCoverage) {}
FalseExecutionCount(FalseExecutionCount), Folded(false) {}
};

/// MCDC Record grouping all information together.
Expand Down Expand Up @@ -702,9 +698,12 @@ struct FunctionRecord {
std::vector<MCDCRecord> MCDCRecords;
/// The number of times this function was executed.
uint64_t ExecutionCount = 0;
bool SingleByteCoverage;

FunctionRecord(StringRef Name, ArrayRef<StringRef> Filenames)
: Name(Name), Filenames(Filenames.begin(), Filenames.end()) {}
FunctionRecord(StringRef Name, ArrayRef<StringRef> Filenames,
bool SingleByteCoverage)
: Name(Name), Filenames(Filenames.begin(), Filenames.end()),
SingleByteCoverage(SingleByteCoverage) {}

FunctionRecord(FunctionRecord &&FR) = default;
FunctionRecord &operator=(FunctionRecord &&) = default;
Expand All @@ -714,11 +713,10 @@ struct FunctionRecord {
}

void pushRegion(CounterMappingRegion Region, uint64_t Count,
uint64_t FalseCount, bool HasSingleByteCoverage) {
uint64_t FalseCount) {
if (Region.Kind == CounterMappingRegion::BranchRegion ||
Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
CountedBranchRegions.emplace_back(Region, Count, FalseCount,
HasSingleByteCoverage);
CountedBranchRegions.emplace_back(Region, Count, FalseCount);
// If both counters are hard-coded to zero, then this region represents a
// constant-folded branch.
if (Region.Count.isZero() && Region.FalseCount.isZero())
Expand All @@ -727,8 +725,7 @@ struct FunctionRecord {
}
if (CountedRegions.empty())
ExecutionCount = Count;
CountedRegions.emplace_back(Region, Count, FalseCount,
HasSingleByteCoverage);
CountedRegions.emplace_back(Region, Count, FalseCount);
}
};

Expand Down
5 changes: 4 additions & 1 deletion llvm/include/llvm/ProfileData/InstrProf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,7 @@ struct InstrProfValueSiteRecord {
/// Profiling information for a single function.
struct InstrProfRecord {
std::vector<uint64_t> Counts;
bool SingleByteCoverage = false;
std::vector<uint8_t> BitmapBytes;

InstrProfRecord() = default;
Expand All @@ -839,13 +840,15 @@ struct InstrProfRecord {
: Counts(std::move(Counts)), BitmapBytes(std::move(BitmapBytes)) {}
InstrProfRecord(InstrProfRecord &&) = default;
InstrProfRecord(const InstrProfRecord &RHS)
: Counts(RHS.Counts), BitmapBytes(RHS.BitmapBytes),
: Counts(RHS.Counts), SingleByteCoverage(RHS.SingleByteCoverage),
BitmapBytes(RHS.BitmapBytes),
ValueData(RHS.ValueData
? std::make_unique<ValueProfData>(*RHS.ValueData)
: nullptr) {}
InstrProfRecord &operator=(InstrProfRecord &&) = default;
InstrProfRecord &operator=(const InstrProfRecord &RHS) {
Counts = RHS.Counts;
SingleByteCoverage = RHS.SingleByteCoverage;
BitmapBytes = RHS.BitmapBytes;
if (!RHS.ValueData) {
ValueData = nullptr;
Expand Down
39 changes: 27 additions & 12 deletions llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,7 @@ Error CoverageMapping::loadFunctionRecord(
else
OrigFuncName = getFuncNameWithoutPrefix(OrigFuncName, Record.Filenames[0]);

bool SingleByteCoverage = ProfileReader.hasSingleByteCoverage();
CounterMappingContext Ctx(Record.Expressions);

std::vector<uint64_t> Counts;
Expand Down Expand Up @@ -855,7 +856,7 @@ Error CoverageMapping::loadFunctionRecord(
return Error::success();

MCDCDecisionRecorder MCDCDecisions;
FunctionRecord Function(OrigFuncName, Record.Filenames);
FunctionRecord Function(OrigFuncName, Record.Filenames, SingleByteCoverage);
for (const auto &Region : Record.MappingRegions) {
// MCDCDecisionRegion should be handled first since it overlaps with
// others inside.
Expand All @@ -873,8 +874,10 @@ Error CoverageMapping::loadFunctionRecord(
consumeError(std::move(E));
return Error::success();
}
Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount,
ProfileReader.hasSingleByteCoverage());
assert(!SingleByteCoverage ||
(0 <= *ExecutionCount && *ExecutionCount <= 1 &&
0 <= *AltExecutionCount && *AltExecutionCount <= 1));
Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);

// Record ExpansionRegion.
if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
Expand Down Expand Up @@ -1270,7 +1273,8 @@ class SegmentBuilder {

/// Combine counts of regions which cover the same area.
static ArrayRef<CountedRegion>
combineRegions(MutableArrayRef<CountedRegion> Regions) {
combineRegions(MutableArrayRef<CountedRegion> Regions,
bool SingleByteCoverage) {
if (Regions.empty())
return Regions;
auto Active = Regions.begin();
Expand All @@ -1297,9 +1301,7 @@ class SegmentBuilder {
// We add counts of the regions of the same kind as the active region
// to handle the both situations.
if (I->Kind == Active->Kind) {
assert(I->HasSingleByteCoverage == Active->HasSingleByteCoverage &&
"Regions are generated in different coverage modes");
if (I->HasSingleByteCoverage)
if (SingleByteCoverage)
Active->ExecutionCount = Active->ExecutionCount || I->ExecutionCount;
else
Active->ExecutionCount += I->ExecutionCount;
Expand All @@ -1311,12 +1313,14 @@ class SegmentBuilder {
public:
/// Build a sorted list of CoverageSegments from a list of Regions.
static std::vector<CoverageSegment>
buildSegments(MutableArrayRef<CountedRegion> Regions) {
buildSegments(MutableArrayRef<CountedRegion> Regions,
bool SingleByteCoverage) {
std::vector<CoverageSegment> Segments;
SegmentBuilder Builder(Segments);

sortNestedRegions(Regions);
ArrayRef<CountedRegion> CombinedRegions = combineRegions(Regions);
ArrayRef<CountedRegion> CombinedRegions =
combineRegions(Regions, SingleByteCoverage);

LLVM_DEBUG({
dbgs() << "Combined regions:\n";
Expand Down Expand Up @@ -1403,10 +1407,14 @@ CoverageData CoverageMapping::getCoverageForFile(StringRef Filename) const {
// the filename, we may get back some records that are not in the file.
ArrayRef<unsigned> RecordIndices =
getImpreciseRecordIndicesForFilename(Filename);
std::optional<bool> SingleByteCoverage;
for (unsigned RecordIndex : RecordIndices) {
const FunctionRecord &Function = Functions[RecordIndex];
auto MainFileID = findMainViewFileID(Filename, Function);
auto FileIDs = gatherFileIDs(Filename, Function);
assert(!SingleByteCoverage ||
*SingleByteCoverage == Function.SingleByteCoverage);
SingleByteCoverage = Function.SingleByteCoverage;
for (const auto &CR : Function.CountedRegions)
if (FileIDs.test(CR.FileID)) {
Regions.push_back(CR);
Expand All @@ -1424,7 +1432,8 @@ CoverageData CoverageMapping::getCoverageForFile(StringRef Filename) const {
}

LLVM_DEBUG(dbgs() << "Emitting segments for file: " << Filename << "\n");
FileCoverage.Segments = SegmentBuilder::buildSegments(Regions);
FileCoverage.Segments =
SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);

return FileCoverage;
}
Expand Down Expand Up @@ -1480,7 +1489,8 @@ CoverageMapping::getCoverageForFunction(const FunctionRecord &Function) const {

LLVM_DEBUG(dbgs() << "Emitting segments for function: " << Function.Name
<< "\n");
FunctionCoverage.Segments = SegmentBuilder::buildSegments(Regions);
FunctionCoverage.Segments =
SegmentBuilder::buildSegments(Regions, Function.SingleByteCoverage);

return FunctionCoverage;
}
Expand All @@ -1490,8 +1500,12 @@ CoverageData CoverageMapping::getCoverageForExpansion(
CoverageData ExpansionCoverage(
Expansion.Function.Filenames[Expansion.FileID]);
std::vector<CountedRegion> Regions;
std::optional<bool> SingleByteCoverage;
for (const auto &CR : Expansion.Function.CountedRegions)
if (CR.FileID == Expansion.FileID) {
assert(!SingleByteCoverage ||
*SingleByteCoverage == Expansion.Function.SingleByteCoverage);
SingleByteCoverage = Expansion.Function.SingleByteCoverage;
Regions.push_back(CR);
if (isExpansion(CR, Expansion.FileID))
ExpansionCoverage.Expansions.emplace_back(CR, Expansion.Function);
Expand All @@ -1503,7 +1517,8 @@ CoverageData CoverageMapping::getCoverageForExpansion(

LLVM_DEBUG(dbgs() << "Emitting segments for expansion of file "
<< Expansion.FileID << "\n");
ExpansionCoverage.Segments = SegmentBuilder::buildSegments(Regions);
ExpansionCoverage.Segments =
SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);

return ExpansionCoverage;
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/ProfileData/InstrProf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,7 @@ void InstrProfRecord::merge(InstrProfRecord &Other, uint64_t Weight,
Value = getInstrMaxCountValue();
Overflowed = true;
}
Counts[I] = Value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deliberate. Even though we only record boolean coverage in the raw profiles, when we aggregate many raw profiles together we can still get some sense of relative hotness by looking at the counter value. Otherwise we lose information if we treat the counter value in the indexed profile as a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't imagine use cases in PGO. I'll leave it unchanged.

In contrast, do you think we could round counters as boolean only in llvm-cov?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense for frontend coverage since we aren't using those values for optimization.

Counts[I] = (SingleByteCoverage && Value != 0 ? 1 : Value);
if (Overflowed)
Warn(instrprof_error::counter_overflow);
}
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/ProfileData/InstrProfReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(

Record.Counts.clear();
Record.Counts.reserve(NumCounters);
Record.SingleByteCoverage = hasSingleByteCoverage();
for (uint32_t I = 0; I < NumCounters; I++) {
const char *Ptr =
CountersStart + CounterBaseOffset + I * getCounterTypeSize();
Expand Down
Loading