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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello. Glad to see this is moving forward! This looks like it's going to be very useful.

Couple of quick, high level questions, in terms of how we expect the interface to be used. If there is a function that currently looks like this:

  bool MergeBlockIntoPredecessor(BasicBlock *BB, DominatorTree *DT = nullptr,
                                 LoopInfo *LI = nullptr,
                                 MemoryDependenceResults *MemDep = nullptr,
                                 DeferredDominance *DDT = nullptr);

.. and we'd like to make it optionally preserve PDT's, do you think it's best to have both an (optional) DT and a DTU? Just the DTU and get DT through it? Would the DTU remain optional or become required? (though possibly empty)

What about something like

  BasicBlock *SplitBlockPredecessors(BasicBlock *BB, ArrayRef<BasicBlock *> Preds,
                                     const char *Suffix,
                                     DominatorTree *DT = nullptr,
                                     LoopInfo *LI = nullptr,
                                     bool PreserveLCSSA = false);

Which I think calls DT->splitBlock() internally. Do you think updating this just to take a DTU instead of a DT, and updating all the call sites is the best way to go?



================
Comment at: include/llvm/IR/DomTreeUpdater.h:138
+
+  /// FLush DomTree updates and return DomTree.
+  /// It also flush out of date updates applied by all holding trees
----------------
Nit: Flush


================
Comment at: include/llvm/IR/DomTreeUpdater.h:142
+  /// It must only be called when it has a DomTree.
+  DominatorTree &flushDomTree();
+
----------------
brzycki wrote:
> I'd actually prefer if flushDomTree() and flushPostDomTree(), and flushAll() were private members. Users shouldn't have to think about flushing state. Instead, have all delections and flushes happen when they request access to a DT or PDT reference such as:
> 
> ```
> auto DTU = DomTreeUpdater(DT, PDT, Lazy);
> ...
> DTU->applyUpdates({Foo, Bar, Insert});
> DTU->applyUpdates({Bar, Baz, Delete});
> ...
> auto DT = DTU->getDT(); // flush() and delete BBs happen here automatically.
> ```
What do you think of having DTU automatically flush remaining updates in it's deconstructor. So that things like these (to steal your examples) work:
  void F(..)
  {
    auto DTU = DomTreeUpdater(DT, PDT, Lazy);
    ...
    DTU->applyUpdates({Foo, Bar, Insert});
    DTU->applyUpdates({Bar, Baz, Delete});
  }

or

  {
    auto DTU = DomTreeUpdater(DT, PDT, Lazy);
    SomeFunctionThatUpdatesTrees(..., DTU, ...);
  }


================
Comment at: include/llvm/IR/DomTreeUpdater.h:201
+  /// trees are up-to-date.
+  void tryFLushDeletedBB();
+
----------------
Nit: Flush


================
Comment at: lib/IR/DomTreeUpdater.cpp:64
+  auto E = PendUpdates.end();
+  assert(I <= E && "Error occurred in DomTreeUpdater::applyLazyUpdate");
+  for (; I != E; ++I) {
----------------
asserts tend to give a filename and linenumber by default, so maybe change this to something describing the error.


https://reviews.llvm.org/D48383