[PATCH] D18610: [PGO] Restore the correct counter value after processing a nested region in case of combined regions.
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 7 09:26:17 PDT 2016
thanks -- you can probably consolidate the merge operation with
'sortNestedRegions'.
David
On Thu, Apr 7, 2016 at 8:54 AM, Igor Kudrin <ikudrin.dev at gmail.com> wrote:
> 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.
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160407/becef274/attachment.html>
More information about the llvm-commits
mailing list