[PATCH] D48383: [Dominators] Add the DomTreeUpdater class

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:164
+    BasicBlock *DelBB = nullptr;
+    function_ref<void(BasicBlock *)> Callback_;
+    void deleted() override {
----------------
Extra line, please


================
Comment at: include/llvm/IR/DomTreeUpdater.h:170
+  };
+  SmallVector<DominatorTree::UpdateType, 16> PendUpdates;
+  size_t PendDTUpdateIndex = 0;
----------------
Extra line, please


================
Comment at: include/llvm/IR/DomTreeUpdater.h:177
+  SmallPtrSet<BasicBlock *, 8> DeletedBBs;
+  SmallVector<CallBackOnDeletion, 8> Callbacks;
+  bool IsRecalculatingDomTree = false;
----------------
Do you expect to have 8 callbacks here in average cases? Once you switch all users of DDT, how many callbacks registered will there be?


================
Comment at: include/llvm/IR/DomTreeUpdater.h:205
+  /// It must only be called when both DT and PDT is held.
+  void recalculateBothTree(Function &F);
+
----------------
nit: s/Tree/Trees


================
Comment at: lib/IR/DomTreeUpdater.cpp:319
+      return;
+  if (Ups == UpdateStrategy::Eager) {
+    if (DT)
----------------
kuhar wrote:
> I'd put an empty line here too
I think you missed this.
Same applies to the ifs below


================
Comment at: lib/IR/DomTreeUpdater.cpp:334
+      return;
+  if (Ups == UpdateStrategy::Eager) {
+    if (DT)
----------------
kuhar wrote:
> Same
Missed?


================
Comment at: lib/IR/DomTreeUpdater.cpp:28
+  const auto Kind = Update.getKind();
+  // Won't affect DomTree and PostDomTree; discard update.
+  if (From == To)
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:31
+    return false;
+  // Discard updates by inspecting the current state of successors of From.
+  // Since isUpdateValid() must be called *after* the Terminator of From is
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:36
+  const bool HasEdge =
+      std::any_of(succ_begin(From), succ_end(From),
+                  [To](const BasicBlock *B) { return B == To; });
----------------
Why not llvm::find?


================
Comment at: lib/IR/DomTreeUpdater.cpp:44
+    return false;
+  // Edge exists in IR.
+  if (Kind == DominatorTree::Delete && HasEdge)
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:47
+    return false;
+  return true;
+}
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:63
+      PendUpdates.begin() + std::max(PendDTUpdateIndex, PendPDTUpdateIndex);
+  auto E = PendUpdates.end();
+  assert(I <= E && "Error occurred in DomTreeUpdater::applyLazyUpdate");
----------------
E should be const


================
Comment at: lib/IR/DomTreeUpdater.cpp:65
+  assert(I <= E && "Error occurred in DomTreeUpdater::applyLazyUpdate");
+  for (; I != E; ++I) {
+    if (Update == *I)
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:68
+      return false; // Discard duplicate updates.
+    if (Invert == *I) {
+      // Update and Invert are both valid (equivalent to a no-op). Remove
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:74
+    }
+  }
+  PendUpdates.push_back(Update); // Save the valid update.
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:106
+    PDT->applyUpdates(ArrayRef<DominatorTree::UpdateType>(
+        PendUpdates.begin() + PendPDTUpdateIndex, PendUpdates.end()));
+    PendPDTUpdateIndex = PendUpdates.size();
----------------
An assert for `begin + index < end` would be welcome


================
Comment at: lib/IR/DomTreeUpdater.cpp:116
+
+bool DomTreeUpdater::hasPendingDeletedBB() const { return !DeletedBBs.empty(); }
+
----------------
I'd move such one-liners to the header


================
Comment at: lib/IR/DomTreeUpdater.cpp:120
+  if (DeletedBBs.empty())
+    return false;
+  for (auto *BB : DeletedBBs) {
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:175
+    return false;
+  if (Strategy == UpdateStrategy::Eager) {
+    DT->recalculate(F);
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:202
+    return false;
+  if (Strategy == UpdateStrategy::Eager) {
+    PDT->recalculate(F);
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:272
+  }
+  DelBB->removeFromParent();
+  eraseDelBBNode(DelBB);
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:282
+      DT->eraseNode(DelBB);
+  if (PDT && !IsRecalculatingPostDomTree)
+    if (PDT->getNode(DelBB))
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:310
+      // on analysis.
+      if (std::none_of(
+              Seen.begin(), Seen.end(),
----------------
Consider using llvm::find


================
Comment at: lib/IR/DomTreeUpdater.cpp:388
+    PendPDTUpdateIndex = PendUpdates.size();
+  const size_t dropIndex = std::min(PendDTUpdateIndex, PendPDTUpdateIndex);
+  PendUpdates.erase(PendUpdates.begin(), PendUpdates.begin() + dropIndex);
----------------
Extra line, please


================
Comment at: lib/IR/DomTreeUpdater.cpp:456
+  }
+  if (PDT) {
+    OS << "Applied but not cleared PostDomTreeUpdates:\n";
----------------
Extra line, please


https://reviews.llvm.org/D48383