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

Chijun Sima via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 27 11:38:03 PDT 2018


NutshellySima added inline comments.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:116
+  /// This function should be called when the class holds a PostDomTree.
+  void recalculatePostDomTree(Function &F);
+  /// Recalculate Both of the trees.
----------------
brzycki wrote:
> NutshellySima wrote:
> > kuhar wrote:
> > > Similar to above.
> > > One more thing: why do we expose these function? Is it possible that someone wants to recalculate just one tree and leave the other one intact? If this is the case, can you explain when it would make sense?
> > As mentioned in the DeferredDomiance document, the DT traversal algorithm needs all BasicBlocks awaiting deletion be deleted before it determines the actual DT. So there should be such recalculate* functions to determine whether to leave the other tree intact (check whether there are BasicBlocks awaiting deletion).
> You might want to add a comment explaining why deleted BBs need to be processed first:
> ```
> /// If there are BasicBlocks pending deletion they must be removed first.
> /// This is necessary because the pending BBs still have their Parent set to F
> /// which is traversed by the tree's recalculate() algorithm and will assert()
> /// if it detects a block in F that is not dominated by entry.
It seems that DT.recalculate() won't assert on this case.


================
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);
----------------
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.


https://reviews.llvm.org/D48383





More information about the llvm-commits mailing list