[PATCH] D48383: [Dominators] Add the DomTreeUpdater class
Jakub Kuderski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 22 00:10:52 PDT 2018
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
More information about the llvm-commits
mailing list