<div dir="ltr">thanks -- you can probably consolidate the merge operation with 'sortNestedRegions'.<div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 7, 2016 at 8:54 AM, Igor Kudrin <span dir="ltr"><<a href="mailto:ikudrin.dev@gmail.com" target="_blank">ikudrin.dev@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks! I'll first prepare a new variant, as David proposed, and then we'll judge, which one is better.<div class="HOEnZb"><div class="h5"><br>
<br>
On 06.04.2016 23:48, Justin Bogner wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Igor Kudrin <<a href="mailto:ikudrin.dev@gmail.com" target="_blank">ikudrin.dev@gmail.com</a>> writes:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
ikudrin updated this revision to Diff 52230.<br>
ikudrin added a comment.<br>
<br>
Thanks a lot for the comments! Please review the new version of the code.<br>
<br>
- Added a new unit test.<br>
- Eliminated dead code.<br>
- Reworked the whole SegmentBuilder class.<br>
- Addressed all other comments.<br>
</blockquote>
LGTM. Thanks!<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="http://reviews.llvm.org/D18610" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18610</a><br>
<br>
Files:<br>
llvm/trunk/include/llvm/ProfileData/CoverageMapping.h<br>
llvm/trunk/lib/ProfileData/CoverageMapping.cpp<br>
llvm/trunk/test/tools/llvm-cov/showTemplateInstantiations.cpp<br>
llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp<br>
<br>
Index: llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp<br>
===================================================================<br>
--- llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp<br>
+++ llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp<br>
@@ -269,6 +269,25 @@<br>
ASSERT_EQ(CoverageSegment(9, 9, false), Segments[3]);<br>
}<br>
+TEST_P(MaybeSparseCoverageMappingTest, restore_combined_counter_after_nested_region) {<br>
+ InstrProfRecord Record("func", 0x1234, { 10, 20, 40 });<br>
+ ProfileWriter.addRecord(std::move(Record));<br>
+ readProfCounts();<br>
+<br>
+ addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);<br>
+ addCMR(Counter::getCounter(1), "file1", 1, 1, 9, 9);<br>
+ addCMR(Counter::getCounter(2), "file1", 3, 3, 5, 5);<br>
+ loadCoverageMapping("func", 0x1234);<br>
+<br>
+ CoverageData Data = LoadedCoverage->getCoverageForFile("file1");<br>
+ std::vector<CoverageSegment> Segments(Data.begin(), Data.end());<br>
+ ASSERT_EQ(4U, Segments.size());<br>
+ ASSERT_EQ(CoverageSegment(1, 1, 30, true), Segments[0]);<br>
+ ASSERT_EQ(CoverageSegment(3, 3, 40, true), Segments[1]);<br>
+ ASSERT_EQ(CoverageSegment(5, 5, 30, false), Segments[2]);<br>
+ ASSERT_EQ(CoverageSegment(9, 9, false), Segments[3]);<br>
+}<br>
+<br>
TEST_P(MaybeSparseCoverageMappingTest, dont_combine_expansions) {<br>
InstrProfRecord Record1("func", 0x1234, {10, 20});<br>
InstrProfRecord Record2("func", 0x1234, {0, 0});<br>
Index: llvm/trunk/test/tools/llvm-cov/showTemplateInstantiations.cpp<br>
===================================================================<br>
--- llvm/trunk/test/tools/llvm-cov/showTemplateInstantiations.cpp<br>
+++ llvm/trunk/test/tools/llvm-cov/showTemplateInstantiations.cpp<br>
@@ -7,10 +7,10 @@<br>
int func(T x) { // ALL-NEXT: 2| [[@LINE]]|int func(T x) {<br>
if(x) // ALL-NEXT: 2| [[@LINE]]| if(x)<br>
return 0; // ALL-NEXT: 1| [[@LINE]]| return 0;<br>
- else // ALL-NEXT: 1| [[@LINE]]| else<br>
+ else // ALL-NEXT: 2| [[@LINE]]| else<br>
return 1; // ALL-NEXT: 1| [[@LINE]]| return 1;<br>
int j = 1; // ALL-NEXT: 0| [[@LINE]]| int j = 1;<br>
-} // ALL-NEXT: 1| [[@LINE]]|}<br>
+} // ALL-NEXT: 2| [[@LINE]]|}<br>
// CHECK: {{^ *(\| )?}}_Z4funcIbEiT_:<br>
// CHECK-NEXT: 1| [[@LINE-9]]|int func(T x) {<br>
Index: llvm/trunk/lib/ProfileData/CoverageMapping.cpp<br>
===================================================================<br>
--- llvm/trunk/lib/ProfileData/CoverageMapping.cpp<br>
+++ llvm/trunk/lib/ProfileData/CoverageMapping.cpp<br>
@@ -273,71 +273,76 @@<br>
class SegmentBuilder {<br>
std::vector<CoverageSegment> Segments;<br>
- SmallVector<const CountedRegion *, 8> ActiveRegions;<br>
+ /// Each entry corresponds to the area of code covered by one or more regions<br>
+ /// with the same bounds. It consists of the first region found for that area<br>
+ /// and the combined counter.<br>
+ SmallVector<std::pair<const CountedRegion *, Optional<uint64_t>>, 8><br>
+ ActiveRegions;<br>
- /// Start a segment with no count specified.<br>
- void startSegment(unsigned Line, unsigned Col) {<br>
- DEBUG(dbgs() << "Top level segment at " << Line << ":" << Col << "\n");<br>
- Segments.emplace_back(Line, Col, /*IsRegionEntry=*/false);<br>
- }<br>
-<br>
- /// Start a segment with the given Region's count.<br>
+ /// Start a segment at the given position with the given count.<br>
void startSegment(unsigned Line, unsigned Col, bool IsRegionEntry,<br>
- const CountedRegion &Region) {<br>
- if (Segments.empty())<br>
- Segments.emplace_back(Line, Col, IsRegionEntry);<br>
- CoverageSegment S = Segments.back();<br>
+ const Optional<uint64_t> &Count) {<br>
// Avoid creating empty regions.<br>
- if (S.Line != Line || S.Col != Col) {<br>
- Segments.emplace_back(Line, Col, IsRegionEntry);<br>
- S = Segments.back();<br>
- }<br>
+ if (!Segments.empty() && Segments.back().Line == Line &&<br>
+ Segments.back().Col == Col)<br>
+ Segments.pop_back();<br>
DEBUG(dbgs() << "Segment at " << Line << ":" << Col);<br>
- // Set this region's count.<br>
- if (Region.Kind != coverage::CounterMappingRegion::SkippedRegion) {<br>
- DEBUG(dbgs() << " with count " << Region.ExecutionCount);<br>
- Segments.back().setCount(Region.ExecutionCount);<br>
- }<br>
+ if (Count.hasValue()) {<br>
+ DEBUG(dbgs() << " with count " << Count.getValue());<br>
+ Segments.emplace_back(Line, Col, Count.getValue(), IsRegionEntry);<br>
+ } else<br>
+ Segments.emplace_back(Line, Col, IsRegionEntry);<br>
DEBUG(dbgs() << "\n");<br>
}<br>
- /// Start a segment for the given region.<br>
- void startSegment(const CountedRegion &Region) {<br>
- startSegment(Region.LineStart, Region.ColumnStart, true, Region);<br>
+ /// Add the region to the top of the active stack and start<br>
+ /// a new segment with the given count.<br>
+ void pushRegion(const CountedRegion *Region,<br>
+ const Optional<uint64_t> &Count) {<br>
+ startSegment(Region->LineStart, Region->ColumnStart, true, Count);<br>
+ ActiveRegions.emplace_back(Region, Count);<br>
}<br>
/// Pop the top region off of the active stack, starting a new segment with<br>
/// the containing Region's count.<br>
void popRegion() {<br>
- const CountedRegion *Active = ActiveRegions.back();<br>
- unsigned Line = Active->LineEnd, Col = Active->ColumnEnd;<br>
+ const CountedRegion *Active = ActiveRegions.back().first;<br>
ActiveRegions.pop_back();<br>
- if (ActiveRegions.empty())<br>
- startSegment(Line, Col);<br>
- else<br>
- startSegment(Line, Col, false, *ActiveRegions.back());<br>
+ Optional<uint64_t> Count = None;<br>
+ if (!ActiveRegions.empty())<br>
+ Count = ActiveRegions.back().second;<br>
+ startSegment(Active->LineEnd, Active->ColumnEnd, false, Count);<br>
}<br>
public:<br>
/// Build a list of CoverageSegments from a sorted list of Regions.<br>
std::vector<CoverageSegment> buildSegments(ArrayRef<CountedRegion> Regions) {<br>
- const CountedRegion *PrevRegion = nullptr;<br>
+ const CountedRegion *ReferenceRegion = nullptr;<br>
+ Optional<uint64_t> Count = None;<br>
for (const auto &Region : Regions) {<br>
+ // Combine counters of the regions which cover the same area.<br>
+ if (ReferenceRegion && ReferenceRegion->startLoc() == Region.startLoc() &&<br>
+ ReferenceRegion->endLoc() == Region.endLoc()) {<br>
+ if (Region.Kind == coverage::CounterMappingRegion::CodeRegion)<br>
+ Count = Count.getValueOr(0) + Region.ExecutionCount;<br>
+ continue;<br>
+ }<br>
+ // Add the previous region with the combined counter to the stack.<br>
+ if (ReferenceRegion)<br>
+ pushRegion(ReferenceRegion, Count);<br>
// Pop any regions that end before this one starts.<br>
while (!ActiveRegions.empty() &&<br>
- ActiveRegions.back()->endLoc() <= Region.startLoc())<br>
+ ActiveRegions.back().first->endLoc() <= Region.startLoc())<br>
popRegion();<br>
- if (PrevRegion && PrevRegion->startLoc() == Region.startLoc() &&<br>
- PrevRegion->endLoc() == Region.endLoc()) {<br>
- if (Region.Kind == coverage::CounterMappingRegion::CodeRegion)<br>
- Segments.back().addCount(Region.ExecutionCount);<br>
- } else {<br>
- // Add this region to the stack.<br>
- ActiveRegions.push_back(&Region);<br>
- startSegment(Region);<br>
- }<br>
- PrevRegion = &Region;<br>
+ ReferenceRegion = &Region;<br>
+ if (Region.Kind != coverage::CounterMappingRegion::SkippedRegion)<br>
+ Count = Region.ExecutionCount;<br>
+ else<br>
+ Count = None;<br>
</blockquote>
I might've written this like so:<br>
<br>
Count = Region.Kind == coverage::CounterMappingRegion::SkippedRegion<br>
? None<br>
: Region.ExecutionCount;<br>
<br>
Up to you though, both ways are fine.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
+ // Add the last region to the stack.<br>
+ if (ReferenceRegion)<br>
+ pushRegion(ReferenceRegion, Count);<br>
// Pop any regions that are left in the stack.<br>
while (!ActiveRegions.empty())<br>
popRegion();<br>
Index: llvm/trunk/include/llvm/ProfileData/CoverageMapping.h<br>
===================================================================<br>
--- llvm/trunk/include/llvm/ProfileData/CoverageMapping.h<br>
+++ llvm/trunk/include/llvm/ProfileData/CoverageMapping.h<br>
@@ -370,13 +370,6 @@<br>
return std::tie(L.Line, L.Col, L.Count, L.HasCount, L.IsRegionEntry) ==<br>
std::tie(R.Line, R.Col, R.Count, R.HasCount, R.IsRegionEntry);<br>
}<br>
-<br>
- void setCount(uint64_t NewCount) {<br>
- Count = NewCount;<br>
- HasCount = true;<br>
- }<br>
-<br>
- void addCount(uint64_t NewCount) { setCount(Count + NewCount); }<br>
};<br>
/// \brief Coverage information to be processed or displayed.<br>
</blockquote></blockquote>
<br>
</div></div></blockquote></div><br></div>