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