[PATCH] D51619: [gcov] Fix branch counters with switch statements

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:630
   bool Result = false;
-  bool InsertIndCounterIncrCode = false;
   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     SmallVector<std::pair<GlobalVariable *, MDNode *>, 8> CountersBySP;
----------------
Reading through the diff I noticed that this `i` is shadowed. But, what is the purpose of this outer loop? Can we just delete it (maybe in a follow-up)?


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:646
+          EdgeToCounter.insert(
+              std::make_pair(std::make_pair(&BB, nullptr), Edges));
           ++Edges;
----------------
Nit, should be cleaner to write insertions as `EdgeToCounter[{&BB, nullptr}] = Edges`.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:649
+        } else {
+          if (int successors = TI->getNumSuccessors()) {
+            for (int i = 0; i != successors; ++i) {
----------------
Nit, might be cleaner to write `for (BasicBlock *Succ : successors(TI)) { ... }`, to avoid nesting / dealing with indices.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:682
+              const unsigned Edge =
+                  EdgeToCounter.lookup(std::make_pair(Pred, &BB));
+              Value *EdgeCounter =
----------------
I suggest using the `find` API and asserting that the returned iterator is valid any time you need to look up an edge index.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:689
+          } else {
+            const unsigned Edge = EdgeToCounter.lookup(
+                std::make_pair(BB.getUniquePredecessor(), &BB));
----------------
The optimizer can take care of eliminating phis with a single incoming value, so it's not essential to perform that optimization here. I recommend folding this into the `EdgeCount >= 2` case above for simplicity.


Repository:
  rL LLVM

https://reviews.llvm.org/D51619