[PATCH] D124490: [InstrProf] Minimal Block Coverage

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

If this new instrumentation mode is for coverage (non-optimization), why modify `PGOUseFunc`? If just to use PGOVerifyBFI, it doesn't justify modifying `PGOInstrumentation`. It would be cleaner to create a new file based on my current understanding.



================
Comment at: llvm/include/llvm/Transforms/Instrumentation/BlockCoverageInference.h:79
+  /// basic block \p Avoid.
+  void getReachableAvoiding(const BasicBlock *Start, const BasicBlock *Avoid,
+                            bool IsForward, BlockSet &Reachable) const;
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:66
+BlockCoverageInference::BlockSet
+BlockCoverageInference::getDependencies(const BasicBlock *BB) const {
+  assert(BB && BB->getParent() == F);
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:103
+
+bool BlockCoverageInference::canFindMinimalCoverage(const Function *F) const {
+  if (F->hasFnAttribute(Attribute::NoReturn))
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:113
+        (void)N;
+  if (std::distance(F->begin(), F->end()) ==
+      std::distance(Visited.begin(), Visited.end()))
----------------
`return F.size() == Visited.size()`


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:120
+void BlockCoverageInference::findDependencies() {
+  assert(!PredecessorDependencies.size() && !SuccessorDependencies.size());
+  // Fallback to instrumenting every block for functions that we cannot run this
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:129
+  auto &EntryBlock = F->getEntryBlock();
+  BlockList ExitBlocks;
+  for (auto &BB : *F)
----------------
BlockList is used only once. Remove `using`


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:131
+  for (auto &BB : *F)
+    if (succ_empty(&BB))
+      ExitBlocks.push_back(&BB);
----------------
This loop can be augmented with more info to include `canFindMinimalCoverage`, then just delete `canFindMinimalCoverage`


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:137
+    BlockSet ReachableFromEntry, ReachableFromExit;
+    getReachableAvoiding(&EntryBlock, &BB, /*IsForward=*/true,
+                         ReachableFromEntry);
----------------
The time complexity is O(|BB|^2) and can be problematic.


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:145
+    bool HasSuperReachablePred =
+        std::any_of(Preds.begin(), Preds.end(), [&](auto *Pred) {
+          return ReachableFromEntry.count(Pred) &&
----------------
llvm::any_of


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:199
+      assert(Path.size() >= 2);
+      return !Path.count(Neighbors.front()) ? Neighbors.front()
+                                            : Neighbors.back();
----------------
`return Path.count(Neighbors[0]) ? Neighbors[1] : Neighbors[0];`


================
Comment at: llvm/lib/Transforms/Instrumentation/BlockCoverageInference.cpp:244
+    for (auto *BB : depth_first_ext(Start, Visited))
+      Reachable.insert(BB);
+  } else {
----------------
Use `llvm::append_range`


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1365
+  bool WasChanged = true;
+  while (WasChanged) {
+    WasChanged = false;
----------------
This can use a flood-fill instead of a fixed-point iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124490