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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:397
@@ +396,3 @@
+/// Combine counts of regions which cover the same area.
+template <class It> static It combineRegionCounts(It First, It Last) {
+  if (First == Last)
----------------
ikudrin wrote:
> davidxl wrote:
> > I think this can be simplified by calling  std::unique
> > 
> > Followed by a scan of the duplicate entries. For each dup entry, call lower_bound to to find the matching entry and update it if it exists.
> I'm afraid I can't fully understand your idea. `std::unique` just removes duplicate entries and provides no way to work with removed items. So, I can't catch, how you suppose to scan dupes. Moreover, `std::lower_bound` requires O(log(N)) comparisons which leads to O(N*log(N)) complexity for the whole method. At the same time, the current code requires only one scan of an array and doesn't require additional dynamic memory.
> Could you provide some code to illustrate your approach?
unique call returns a iterator pointing to the new logical end of the vector. The dups are from logical end to the original end. You are right that lower_bound O(log(N)), but the overall complexity is O(N*log(N)) only when the # of dups is linear to the total number of regions.  I doubt it is the case in real case, but your concern is legit.

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:398
@@ +397,3 @@
+template <class It> static It combineRegionCounts(It First, It Last) {
+  if (First == Last)
+    return Last;
----------------
Last --> End (which is one past last)

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:403
@@ +402,3 @@
+  for (++I; I != Last; ++I) {
+    if (Active->startLoc() != I->startLoc() ||
+        Active->endLoc() != I->endLoc()) {
----------------
Can you just do

// Find a new region
if (Active->StartLoc() != ... ) {
   Active = I;
   continue;
}

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:411
@@ +410,3 @@
+    }
+    if (I->Kind != coverage::CounterMappingRegion::CodeRegion)
+      continue;
----------------
Add a comment before this line -- "// merge duplicate region"

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:487
@@ -469,1 +486,3 @@
   sortNestedRegions(Regions.begin(), Regions.end());
+  Regions.erase(combineRegionCounts(Regions.begin(), Regions.end()),
+                Regions.end());
----------------
ikudrin wrote:
> davidxl wrote:
> > This probably can be moved into 'sortNestedRegions'.
> As they are separate and mostly independent tasks, I don't think it's good to merge them in one function. On the other hand, we can move both `sortNestedRegions` and `combineRegionCounts` into the `SegmentBuilder` class and simplify callers this way.
yes -- since sortNestedRegions and combineRegions are always called together side by side -- having a wrapper call will be nicer.


http://reviews.llvm.org/D18610