[PATCH] D124490: [InstrProf] Minimal Block Coverage

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:111
+  for (auto &BB : F)
+    if (succ_empty(&BB))
+      ExitBlocks.push_back(&BB);
----------------
MaskRay wrote:
> Rename ExitBlocks: BB may end with an unreachable/resume/cleanupret/etc. I think we don't name them exit blocks.
Is `TerminalBlocks` a better name?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1365
+  bool WasChanged = true;
+  while (WasChanged) {
+    WasChanged = false;
----------------
MaskRay wrote:
> ellis wrote:
> > MaskRay wrote:
> > > This can use a flood-fill instead of a fixed-point iteration.
> > I opted to use this algorithm for simplicity. Since most CFGs are relatively small, this function doesn't contribute too much to buildtime. If we discover that this causes a slowdown in some real world scenario then we can improve it. In fact, we can use info from the block coverage inference algorithm to infer all blocks in one pass. But again, this would add complexity and I would rather not do this unless we needed to.
> I think it's worth fixing it. A flood-fill algorithm is nearly of the same length and avoids the performance pitfall. That style is used much more than the iterative algorithm here.
I've changes this to a flood-fill algorithm, but I needed to first compute `InverseDependencies` which is done in one pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124490