[PATCH] D93073: [llvm-cov gcov] Replace Donald B. Johnson's cycle enumeration with iterative cycle finding

[Xinhao Yuan via Phabricator via llvm-commits]

xinhaoyuan added inline comments.


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:428
+uint64_t
+GCOVBlock::augmentOneCycle(GCOVBlock *u,
+                           std::vector<std::pair<GCOVBlock *, size_t>> &stack) {
----------------
nitnit: without the context of network-flow it looks like the function is "reducing" or "cancelling" a cycle. maybe fine to keep it.


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:433
+  stack.emplace_back(u, 0);
+  u->incoming = (GCOVArc *)1; // Mark u available for cycle detection
+  for (;;) {
----------------
Is it possible to have self-arc? If so this pointer would be de-referenced below.


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:435
+  for (;;) {
+    std::tie(u, i) = stack.back();
+    if (i == u->succ.size()) {
----------------
nit: maybe not reuse the argument variable? it would also help avoid de-referencing the special pointer value by matching block pointer with the start block.


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:453
     }
-
-    Path.pop_back();
-  }
-
-  if (FoundCircuit) {
-    GCOVBlock::unblock(V, Blocked, BlockLists);
-  } else {
-    for (auto E : V->dsts()) {
-      const GCOVBlock *W = &E->dst;
-      if (W < Start || find(Blocks, W) == Blocks.end()) {
-        continue;
-      }
-      const size_t index = find(Blocked, W) - Blocked.begin();
-      BlockVector &List = BlockLists[index];
-      if (find(List, V) == List.end()) {
-        List.push_back(V);
-      }
+    uint64_t flow = succ->cycleCount;
+    for (GCOVBlock *v = u;;) {
----------------
nit: maybe MinCount? this seems a bit off without the context of network-flow.


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:453-466
+    uint64_t flow = succ->cycleCount;
+    for (GCOVBlock *v = u;;) {
+      flow = std::min(flow, v->incoming->cycleCount);
+      v = &v->incoming->src;
+      if (v == &succ->dst)
+        break;
     }
----------------
xinhaoyuan wrote:
> nit: maybe MinCount? this seems a bit off without the context of network-flow.
the control flow of the loops seems duplicated yet tricky. would it be more readable to first extract the path into a vector and apply simple loops on it? 


================
Comment at: llvm/lib/ProfileData/GCOV.cpp:467
+    }
+    return flow;
   }
----------------
when returned with a cycle found. "incoming" of the blocks on the cycle would not be cleared. can it lead to undefined behaviors in the subsequent traversal?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93073/new/

https://reviews.llvm.org/D93073