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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Thank you for continuing this work @NutshellySima .

> 1. Make DomTreeUpdater flush() on deconstruction.

Excellent, this is a nice convenience.

> 2. Add insertEdgeRelaxed() and deleteEdgeRelaxed() to cover the behavior of the original DeferredDominance's insertEdge()/deleteEdge().

Maybe it'd be better to have a default boolean third variable?
`deleteEdge(BasicBlock *From, BasicBlock *To, bool Relaxed = false) { ...`



================
Comment at: include/llvm/IR/DomTreeUpdater.h:182
+  /// Remove all the instrcutions of DelBB and make sure DelBB has a valid
+  /// terminator instruction. Assert if DelBB is nullptr or has predecessors.
+  void validateDeleteBB(BasicBlock *DelBB);
----------------
NutshellySima wrote:
> brzycki wrote:
> > It'd be good to mention why this is necessary in the comment. When immediately deleting a BB, the call `removeFromParent()` removes all its instructions as well. However, in the deferred case the block isn't really deleted and is still a child of F. This causes problems for algorithms that do `for (auto BB: F)` and expect terminator instructions, instruction Uses, and REPL to all be consistent for every block inside a function.  Without these fixups all sorts of routines will start asserting() complaining that the state of the IR is inconsistent.
> It seems that calling `removeFromParent()` does not remove all its instructions.
Correction, I meant `llvm::DeleteDeadBlock()`. You'll see a similar loop to `DDT->deleteBB()` which does a RAUW to undef for all instructions and then calls `removeFromParent()`. The biggest change that `deleteBB()` does is adds a valid `TI`: necessary for llvm to keep the block as a child of `F()`.


https://reviews.llvm.org/D48383