[PATCH] D18610: [PGO] Restore the correct counter value after processing a nested region in case of combined regions.
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 6 10:48:28 PDT 2016
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