[Coverage] Move SingleByteCoverage out of CountedRegion

[chapuni]

SingleByteCoverage is not per-region attribute at least.

At the moment, this change moves it into FunctionRecord.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: NAKAMURA Takumi (chapuni)

Changes

SingleByteCoverage is not per-region attribute at least.

At the moment, this change moves it into FunctionRecord.

---

Full diff: https://github.com/llvm/llvm-project/pull/110966.diff

2 Files Affected:

• (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+12-15)
• (modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+24-12)

diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index fa07b3a9e8b14f..77c447efe1a7c9 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -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.
@@ -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;
@@ -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())
@@ -727,8 +725,7 @@ struct FunctionRecord {
     }
     if (CountedRegions.empty())
       ExecutionCount = Count;
-    CountedRegions.emplace_back(Region, Count, FalseCount,
-                                HasSingleByteCoverage);
+    CountedRegions.emplace_back(Region, Count, FalseCount);
   }
 };
 
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 18643c6b44485e..a02136d5b0386d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -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;
@@ -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.
@@ -873,8 +874,7 @@ Error CoverageMapping::loadFunctionRecord(
       consumeError(std::move(E));
       return Error::success();
     }
-    Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount,
-                        ProfileReader.hasSingleByteCoverage());
+    Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);
 
     // Record ExpansionRegion.
     if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
@@ -1270,7 +1270,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();
@@ -1297,9 +1298,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;
@@ -1311,12 +1310,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";
@@ -1403,10 +1404,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);
@@ -1424,7 +1429,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;
 }
@@ -1480,7 +1486,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;
 }
@@ -1490,8 +1497,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);
@@ -1503,7 +1514,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;
 }

[ellishg]

Looks good overall. I assume if you added a test case in #110972 that would also cover this code.

[ellishg]

This will crash if RecordIndices is empty. Is that possible? If not, can we add an assert?

[ellishg]

Again, this will crash if Expansion.Function.CountedRegions is empty.