[Coverage] Move SingleByteCoverage out of CountedRegion #110966

chapuni · 2024-10-03T07:08:52Z

SingleByteCoverage is not per-region attribute at least.
Move it into CoverageData since it comes from profdata.

Depends on: #120841

`SingleByteCoverage` is not per-region attribute at least. At the moment, this change moves it into `FunctionRecord`.

llvmbot · 2024-10-03T07:09:32Z

@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;
 }

- Round `Counts` as 1/0 - Confirm both `ExecutionCount` and `AltExecutionCount` are in range.

…tedRegion")

ellishg

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

ellishg · 2024-10-09T00:10:46Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+  FileCoverage.Segments =
+      SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);


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

ellishg · 2024-10-09T00:11:12Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+  ExpansionCoverage.Segments =
+      SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);


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

…matches

And reformat. NFC.

…ingle/merge

Conflicts: llvm/test/tools/llvm-cov/branch-macros.cpp

…ingle/merge Conflicts: llvm/test/tools/llvm-cov/Inputs/showLineExecutionCounts.cpp

…v/binary' into users/chapuni/cov/single/refactor-base Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test

ornata

I think at this point this looks fine? @ellishg ?

chapuni · 2024-12-23T05:03:29Z

@ellishg I rewrote this based on #120841.
SingleByteCoverage is passed to llvm-cov via CoverageData.

Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test

…/refactor

Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test llvm/tools/llvm-cov/CodeCoverage.cpp llvm/tools/llvm-cov/SourceCoverageView.h

[Coverage] Move SingleByteCoverage out of CountedRegion

178b57c

`SingleByteCoverage` is not per-region attribute at least. At the moment, this change moves it into `FunctionRecord`.

chapuni requested review from ornata, gulfemsavrun and ellishg October 3, 2024 07:08

llvmbot added the PGO Profile Guided Optimizations label Oct 3, 2024

chapuni added 2 commits October 3, 2024 16:23

[Coverage] Make SingleByteCoverage work consistent to merging

aacb50d

- Round `Counts` as 1/0 - Confirm both `ExecutionCount` and `AltExecutionCount` are in range.

Rework. (Also reverts "[Coverage] Move SingleByteCoverage out of Coun…

b9bbc7c

…tedRegion")

chapuni mentioned this pull request Oct 5, 2024

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

Merged

ellishg reviewed Oct 9, 2024

View reviewed changes

chapuni added 7 commits October 21, 2024 09:21

clang/test/CoverageMapping/single-byte-counters.cpp: Rewrite counter …

52f072e

…matches

test/llvm-cov: Transform %.c* tests to {%.test, Inputs/%.c*}

97a4a8f

And reformat. NFC.

Introduce test/llvm-cov/Inputs/yaml.makefile for convenience.

c50c492

Add tests for SingleByteCoverage

d7c5b44

Merge branch 'users/chapuni/cov/single/test' into users/chapuni/cov/s…

6675226

…ingle/merge

Fix a test to fix linecount=1

5fc3408

threads.c: Fixup on the clean testdir

eb7fff9

chapuni mentioned this pull request Nov 21, 2024

Single byte coverage with branch coverage #113115

Draft

chapuni added 9 commits November 21, 2024 23:27

Rename threads.c to threads.test since it is no longer C source file.

7543095

llvm/test/tools/llvm-cov/Inputs: Avoid wildcards rm -rf %t*.dir

00ac90d

Merge branch 'main' into users/chapuni/cov/single/test

fcb3ee8

Conflicts: llvm/test/tools/llvm-cov/branch-macros.cpp

Use [[#min(C,n)]] for tests

5fa862a

llvm-cov: Introduce --binary-counters

805e9a9

Merge branch 'users/chapuni/cov/single/test' into users/chapuni/cov/s…

68d7b3b

…ingle/merge Conflicts: llvm/test/tools/llvm-cov/Inputs/showLineExecutionCounts.cpp

Update tests

24457a7

Merge branches 'users/chapuni/cov/single/merge' and 'users/chapuni/co…

805dbd9

…v/binary' into users/chapuni/cov/single/refactor-base Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test

New SingleByteCoverage

f96b435

chapuni changed the base branch from main to users/chapuni/cov/single/refactor-base December 21, 2024 17:15

threads.c => threads.test (following #113114)

47550d1

ornata approved these changes Dec 23, 2024

View reviewed changes

chapuni added 3 commits December 23, 2024 16:03

s/Count1/BinaryCount/

f4dc4eb

Update desc

822620b

Merge branch 'main' into users/chapuni/cov/binary

658bd48

Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test

chapuni changed the base branch from users/chapuni/cov/single/refactor-base to users/chapuni/cov/binary December 24, 2024 06:43

chapuni added 4 commits December 24, 2024 15:45

Merge branch 'users/chapuni/cov/binary' into users/chapuni/cov/single…

0780993

…/refactor

Fix wrong merge resolutions

dfc99ba

Reorganize CoverageMapping::SingleByteCoverage

f3c9593

Prune commented-out line

3780e07

This was referenced Dec 27, 2024

[Coverage] Sort MCDCRecord::ExecVectors order by Bitmap index #121195

Open

llvm-cov: Merge records for template instantiations #121197

Draft

Base automatically changed from users/chapuni/cov/binary to main December 27, 2024 10:48

Merge branch 'main' into users/chapuni/cov/single/refactor

894c383

Conflicts: llvm/test/tools/llvm-cov/branch-macros.test llvm/test/tools/llvm-cov/showLineExecutionCounts.test llvm/tools/llvm-cov/CodeCoverage.cpp llvm/tools/llvm-cov/SourceCoverageView.h

chapuni merged commit aa2fdc6 into main Dec 27, 2024
6 of 8 checks passed

chapuni deleted the users/chapuni/cov/single/refactor branch December 27, 2024 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Coverage] Move SingleByteCoverage out of CountedRegion #110966

[Coverage] Move SingleByteCoverage out of CountedRegion #110966

Uh oh!

chapuni commented Oct 3, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 3, 2024

Uh oh!

ellishg left a comment

Uh oh!

ellishg Oct 9, 2024

Uh oh!

ellishg Oct 9, 2024

Uh oh!

ornata left a comment

Uh oh!

chapuni commented Dec 23, 2024

Uh oh!

Uh oh!

Uh oh!

		FileCoverage.Segments =
		SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);

		ExpansionCoverage.Segments =
		SegmentBuilder::buildSegments(Regions, *SingleByteCoverage);

[Coverage] Move SingleByteCoverage out of CountedRegion #110966

[Coverage] Move SingleByteCoverage out of CountedRegion #110966

Uh oh!

Conversation

chapuni commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 3, 2024

Uh oh!

ellishg left a comment

Choose a reason for hiding this comment

Uh oh!

ellishg Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

ellishg Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

ornata left a comment

Choose a reason for hiding this comment

Uh oh!

chapuni commented Dec 23, 2024

Uh oh!

Uh oh!

Uh oh!

chapuni commented Oct 3, 2024 •

edited

Loading