[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