[PATCH] D49560: Add support to track total counts for DebugCounter
George Burgess IV via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 19 14:33:54 PDT 2018
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
More information about the llvm-commits
mailing list