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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In https://reviews.llvm.org/D48383#1140675, @dmgreen wrote:

> 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)


The idea is to have it only take DTU (by reference (non-optional)), such that MergeBlockIntoPredecessor preserves whatever you pass it (possibly no tree).

> 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?

In general. the preferred way would be take DTU as an argument and try to use the incremental api.
There are some cases where it seems like you can do some smarter things with more low-level operations, e.g. changing the function entry block, splitting predecessor, but from what I know there are only a couple of them. In such cases we can expose them as dedicated functions in DTU.


https://reviews.llvm.org/D48383