[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
Wed Apr 20 14:22:55 PDT 2016
Igor Kudrin <ikudrin.dev at gmail.com> writes:
> ikudrin updated this revision to Diff 54217.
> ikudrin added a comment.
>
> - Rebased to the top.
> - Moved `sortNestedRegions` and `combineRegions` into `SegmentBuilder`.
> - `buildSegments` now receives `MutableArrayRef` and performs sorting
> and combining regions internally.
> - Some `ASSERT_EQ` was changed to `EXPECT_EQ` in the added test.
>
>
> 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,70 @@
> }
>
> 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 I = Regions.begin();
> + auto End = Regions.end();
> + for (++I; I != End; ++I) {
I would probably declare I as `Regions.begin() + 1` or `Active + 1` in
the init-statement of the for loop, rather than declaring it out of the
loop and then incrementing it.
> + 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)
> + continue;
> + if (Active->Kind == coverage::CounterMappingRegion::SkippedRegion)
> + *Active = *I;
> + else
> + Active->ExecutionCount += I->ExecutionCount;
I find this part a little hard to read. Can you add a comment explaining
what the cases we care about here are doing?
It might also make sense to order this the other way, so that the case
where we update Active's ExecutionCount comes first and the three
different ways we continue to iterate follow. This would probably make
the logic easier to follow since it would have less "not"s in it. If
that ends up being worse though this is fine.
> + }
> + 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 +429,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 +448,6 @@
> }
> }
>
> - sortNestedRegions(Regions.begin(), Regions.end());
> DEBUG(dbgs() << "Emitting segments for file: " << Filename << "\n");
> FileCoverage.Segments = SegmentBuilder::buildSegments(Regions);
>
> @@ -469,7 +489,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 +507,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