[PATCH] D50031: Add pass to print out DebugCounter info

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!

I don't have a strong feeling for what the right place for this is (e.g. in IR/, ...), and I'm not confident enough with our pass infra and such as a whole to stamp this. I dunno how familiar davide is with these bits; if "not very," we may have to search for someone to poke for a LGTM.

Happily, none of this keeps me from my hobby: nitpicking. :)



================
Comment at: include/llvm/IR/DebugCounterPrinter.h:1
+//===- DebugCounterPrinter.h - DebugCounterPrinter -------------*- C++ -*-===//
+//
----------------
Nit: Looks like we're missing a '-' to bring this to 80 characters.

(One of the few benefits of having a skinny monitor: wrapping makes things like this a bit more obvious...)


================
Comment at: lib/Support/DebugCounter.cpp:105
 void DebugCounter::print(raw_ostream &OS) const {
-  OS << "Counters and values:\n";
+  SmallVector<StringRef, 16> CounterNames;
   for (const auto &KV : Counters)
----------------
nit: Looks like we might be able to shrink this to `CounterNames(RegisteredCounters.begin(), RegisteredCounters.end());` and drop the loop below?

It's a bit unfortunate that the `std::map` isn't directly accessible in RegisteredCounters, since that would give us exactly what we're looking for without this intermediate vector. I'm OK with having the vector, though, since this'll only be run for debugging purposes anyway...


================
Comment at: lib/Support/DebugCounter.cpp:109
+
+  llvm::sort(CounterNames.begin(), CounterNames.end());
+
----------------
nit: please remove the `llvm::` (unless it's necessary?)


================
Comment at: lib/Support/DebugCounter.cpp:113
+  OS << "Counters and values:\n";
+  for (const auto CounterName : CounterNames) {
+    unsigned CounterID = getCounterId(CounterName);
----------------
Nit: style guide [says](https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto) to use `const auto &` unless you need a copy, which I don't think we do.


Repository:
  rL LLVM

https://reviews.llvm.org/D50031