[PATCH] D49560: Add support to track total counts for DebugCounter

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a reviewer: davide.
george.burgess.iv added a comment.

Thanks for this!

Looks like these were added in https://reviews.llvm.org/D29998, so I'm tagging davide, who reviewed that, in case there's context/history I'm missing.



================
Comment at: include/llvm/Support/DebugCounter.h:81
+      auto &CounterInfo = Result->second;
+      // Record how many times does shouldExecute() run for this Counter
+      ++CounterInfo.Count;
----------------
nit: This comment sort of feels redundant to me.


================
Comment at: include/llvm/Support/DebugCounter.h:93
         return true;
-      if (CounterPair.second != 0) {
-        --CounterPair.second;
+      if (CounterInfo.StopAfter + CounterInfo.Skip > CounterInfo.Count)
         return true;
----------------
Should this be `>= Count`?

e.g. if I ask for skip=1 + stop-after=1, we'll end up having `Count == 2` on the second call to `shouldExecute` (which should return `true`).


================
Comment at: include/llvm/Support/DebugCounter.h:94
+      if (CounterInfo.StopAfter + CounterInfo.Skip > CounterInfo.Count)
         return true;
       return false;
----------------
nit: this can be folded into `return CounterInfo.StopAfter + CounterInfo.Skip > CounterInfo.Count;`


================
Comment at: include/llvm/Support/DebugCounter.h:153
+    CounterInfo CI;
+    Counters[Result] = CI;
     return Result;
----------------
nit: can this just be `Counters[Result] = {};`?


================
Comment at: include/llvm/Support/DebugCounter.h:162
+  };
+  DenseMap<unsigned, CounterInfo> Counters;
   DenseMap<unsigned, std::string> CounterDesc;
----------------
I don't know the history of this code, but it seems reasonable to me to make this a `vector`, add a `string Desc` to `CounterInfo`, and drop `CounterDesc` entirely. `CounterVector` just returns `[1..UINT_MAX]`, in order, on insertion anyway.

(Alternatively, it seems that we could replace all three of these containers with a single `StringMap<CounterInfo>` and hand `CounterInfo *`s out instead of `unsigned`s, but that starts to stray pretty far from the goal of this patch, so... :) )


Repository:
  rL LLVM

https://reviews.llvm.org/D49560