[PATCH] D18610: [PGO] Restore the correct counter value after processing a nested region in case of combined regions.

[Justin Bogner via llvm-commits]

Igor Kudrin <ikudrin.dev at gmail.com> writes:
> ikudrin created this revision.
> ikudrin added reviewers: bogner, davidxl.
> ikudrin added a subscriber: llvm-commits.
>
> If we have several counters which cover the same code region, we should restore
> the combined counter value for that region when return from a nested region.

Please write a test for this explicitly - I don't think that just
modifying showTemplateInstantiations is sufficient coverage. A unittest
is probably best (see unittests/ProfileData/CoverageMappingTest for
examples).

> http://reviews.llvm.org/D18610
>
> Files:
>   llvm/trunk/lib/ProfileData/CoverageMapping.cpp
>   llvm/trunk/test/tools/llvm-cov/showTemplateInstantiations.cpp
>
> 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,7 +273,7 @@
>  
>  class SegmentBuilder {
>    std::vector<CoverageSegment> Segments;
> -  SmallVector<const CountedRegion *, 8> ActiveRegions;
> +  SmallVector<std::pair<const CountedRegion *, unsigned>, 8> ActiveRegions;

Please add a comment explaining that the `unsigned` here is an index
into the segment list.

>  
>    /// Start a segment with no count specified.
>    void startSegment(unsigned Line, unsigned Col) {
> @@ -283,7 +283,7 @@
>  
>    /// Start a segment with the given Region's count.
>    void startSegment(unsigned Line, unsigned Col, bool IsRegionEntry,
> -                    const CountedRegion &Region) {
> +                    const Optional<uint64_t> &Count) {

You'll need to update the doc comment now that there isn't a "given
Region".

>      if (Segments.empty())
>        Segments.emplace_back(Line, Col, IsRegionEntry);
>      CoverageSegment S = Segments.back();
> @@ -294,28 +294,35 @@
>      }
>      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.back().setCount(Count.getValue());
>      }
>      DEBUG(dbgs() << "\n");
>    }
>  
>    /// Start a segment for the given region.
>    void startSegment(const CountedRegion &Region) {
> -    startSegment(Region.LineStart, Region.ColumnStart, true, Region);
> +    if (Region.Kind != coverage::CounterMappingRegion::SkippedRegion)
> +      startSegment(Region.LineStart, Region.ColumnStart, true,
> +                   Region.ExecutionCount);
> +    else
> +      startSegment(Region.LineStart, Region.ColumnStart, true, None);
>    }
>  
>    /// 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();
> +    const CountedRegion *Active = ActiveRegions.back().first;
>      unsigned Line = Active->LineEnd, Col = Active->ColumnEnd;
>      ActiveRegions.pop_back();
> -    if (ActiveRegions.empty())
> -      startSegment(Line, Col);
> -    else
> -      startSegment(Line, Col, false, *ActiveRegions.back());
> +    Optional<uint64_t> Count;

It's probably clearer to say "Count = None" here.

> +    if (!ActiveRegions.empty()) {
> +      const CoverageSegment &Segment = Segments[ActiveRegions.back().second];
> +      if (Segment.HasCount)
> +        Count = Segment.Count;
> +    }
> +    startSegment(Line, Col, false, Count);
>    }
>  
>  public:
> @@ -325,15 +332,15 @@
>      for (const auto &Region : Regions) {
>        // 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);
> +        ActiveRegions.emplace_back(&Region, Segments.size());
>          startSegment(Region);
>        }
>        PrevRegion = &Region;