[PATCH] D93073: [llvm-cov gcov] Replace Donald B. Johnson's cycle enumeration with iterative cycle finding
Xinhao Yuan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 10 18:17:33 PST 2020
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
More information about the llvm-commits
mailing list