[PATCH] D18610: [Coverage] Restore the correct count value after processing a nested region in case of combined regions.
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 22 10:57:28 PDT 2016
Igor Kudrin <ikudrin.dev at gmail.com> writes:
> ikudrin updated this revision to Diff 54664.
> ikudrin added a comment.
>
> - Reworked initialization of `I` according to Justin's comment.
> - Added some explanation comments for the "Merge duplicate region" code block.
>
> Please note that I've tried to make the behavior of the new code as
> close to the original as possible, apart from changes required for fix
> the described issue. More changes are coming, which will also change
> this block of code, so it's anyway not the final variant. See
> http://reviews.llvm.org/D18831.
Sounds good. LGTM.
>
> http://reviews.llvm.org/D18610
>
> Files:
> include/llvm/ProfileData/CoverageMapping.h
> lib/ProfileData/CoverageMapping.cpp
> test/tools/llvm-cov/showTemplateInstantiations.cpp
> unittests/ProfileData/CoverageMappingTest.cpp
>
> Index: unittests/ProfileData/CoverageMappingTest.cpp
> ===================================================================
> --- unittests/ProfileData/CoverageMappingTest.cpp
> +++ unittests/ProfileData/CoverageMappingTest.cpp
> @@ -402,6 +402,26 @@
> 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));
> +
> + startFunction("func", 0x1234);
> + 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();
> +
> + CoverageData Data = LoadedCoverage->getCoverageForFile("file1");
> + std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
> + ASSERT_EQ(4U, Segments.size());
> + EXPECT_EQ(CoverageSegment(1, 1, 30, true), Segments[0]);
> + EXPECT_EQ(CoverageSegment(3, 3, 40, true), Segments[1]);
> + EXPECT_EQ(CoverageSegment(5, 5, 30, false), Segments[2]);
> + EXPECT_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: test/tools/llvm-cov/showTemplateInstantiations.cpp
> ===================================================================
> --- test/tools/llvm-cov/showTemplateInstantiations.cpp
> +++ 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: lib/ProfileData/CoverageMapping.cpp
> ===================================================================
> --- lib/ProfileData/CoverageMapping.cpp
> +++ lib/ProfileData/CoverageMapping.cpp
> @@ -286,20 +286,17 @@
> /// Start a segment with the given Region's 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();
> // 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);
> - }
> + Segments.emplace_back(Line, Col, Region.ExecutionCount, IsRegionEntry);
> + } else
> + Segments.emplace_back(Line, Col, IsRegionEntry);
> DEBUG(dbgs() << "\n");
> }
>
> @@ -321,35 +318,73 @@
> }
>
> void buildSegmentsImpl(ArrayRef<CountedRegion> Regions) {
> - const CountedRegion *PrevRegion = nullptr;
> for (const auto &Region : Regions) {
> // Pop any regions that end before this one starts.
> while (!ActiveRegions.empty() &&
> ActiveRegions.back()->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;
> + // Add this region to the stack.
> + ActiveRegions.push_back(&Region);
> + startSegment(Region);
> }
> // Pop any regions that are left in the stack.
> while (!ActiveRegions.empty())
> popRegion();
> }
>
> + /// Sort a nested sequence of regions from a single file.
> + static void sortNestedRegions(MutableArrayRef<CountedRegion> Regions) {
> + std::sort(Regions.begin(), Regions.end(),
> + [](const CountedRegion &LHS, const CountedRegion &RHS) {
> + if (LHS.startLoc() == RHS.startLoc())
> + // When LHS completely contains RHS, we sort LHS first.
> + return RHS.endLoc() < LHS.endLoc();
> + return LHS.startLoc() < RHS.startLoc();
> + });
> + }
> +
> + /// Combine counts of regions which cover the same area.
> + static ArrayRef<CountedRegion>
> + combineRegions(MutableArrayRef<CountedRegion> Regions) {
> + if (Regions.empty())
> + return Regions;
> + auto Active = Regions.begin();
> + auto End = Regions.end();
> + for (auto I = Regions.begin() + 1; I != End; ++I) {
> + if (Active->startLoc() != I->startLoc() ||
> + Active->endLoc() != I->endLoc()) {
> + // Shift to the next region.
> + ++Active;
> + if (Active != I)
> + *Active = *I;
> + continue;
> + }
> + // Merge duplicate region.
> + if (I->Kind != coverage::CounterMappingRegion::CodeRegion)
> + // Add counts only from CodeRegions.
> + continue;
> + if (Active->Kind == coverage::CounterMappingRegion::SkippedRegion)
> + // We have to overwrite SkippedRegions because of special handling
> + // of them in startSegment().
> + *Active = *I;
> + else
> + // Otherwise, just append the count.
> + Active->ExecutionCount += I->ExecutionCount;
> + }
> + return Regions.drop_back(std::distance(++Active, End));
> + }
> +
> public:
> - /// Build a list of CoverageSegments from a sorted list of Regions.
> + /// Build a list of CoverageSegments from a list of Regions.
> static std::vector<CoverageSegment>
> - buildSegments(ArrayRef<CountedRegion> Regions) {
> + buildSegments(MutableArrayRef<CountedRegion> Regions) {
> std::vector<CoverageSegment> Segments;
> SegmentBuilder Builder(Segments);
> - Builder.buildSegmentsImpl(Regions);
> +
> + sortNestedRegions(Regions);
> + ArrayRef<CountedRegion> CombinedRegions = combineRegions(Regions);
> +
> + Builder.buildSegmentsImpl(CombinedRegions);
> return Segments;
> }
> };
> @@ -397,17 +432,6 @@
> return None;
> }
>
> -/// Sort a nested sequence of regions from a single file.
> -template <class It> static void sortNestedRegions(It First, It Last) {
> - std::sort(First, Last,
> - [](const CountedRegion &LHS, const CountedRegion &RHS) {
> - if (LHS.startLoc() == RHS.startLoc())
> - // When LHS completely contains RHS, we sort LHS first.
> - return RHS.endLoc() < LHS.endLoc();
> - return LHS.startLoc() < RHS.startLoc();
> - });
> -}
> -
> static bool isExpansion(const CountedRegion &R, unsigned FileID) {
> return R.Kind == CounterMappingRegion::ExpansionRegion && R.FileID == FileID;
> }
> @@ -427,7 +451,6 @@
> }
> }
>
> - sortNestedRegions(Regions.begin(), Regions.end());
> DEBUG(dbgs() << "Emitting segments for file: " << Filename << "\n");
> FileCoverage.Segments = SegmentBuilder::buildSegments(Regions);
>
> @@ -469,7 +492,6 @@
> FunctionCoverage.Expansions.emplace_back(CR, Function);
> }
>
> - sortNestedRegions(Regions.begin(), Regions.end());
> DEBUG(dbgs() << "Emitting segments for function: " << Function.Name << "\n");
> FunctionCoverage.Segments = SegmentBuilder::buildSegments(Regions);
>
> @@ -488,7 +510,6 @@
> ExpansionCoverage.Expansions.emplace_back(CR, Expansion.Function);
> }
>
> - sortNestedRegions(Regions.begin(), Regions.end());
> DEBUG(dbgs() << "Emitting segments for expansion of file " << Expansion.FileID
> << "\n");
> ExpansionCoverage.Segments = SegmentBuilder::buildSegments(Regions);
> Index: include/llvm/ProfileData/CoverageMapping.h
> ===================================================================
> --- include/llvm/ProfileData/CoverageMapping.h
> +++ 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