[PATCH] D35391: [Dominators] Teach LoopDeletion to use the new incremental API

[Tobias Grosser via Phabricator via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi @kuhar,

thank you for updating the comment. I found a couple of minor typos. The code likely looks good, but I don't see any reference to the irreducible control flow issues you mentioned. I assume you are now connecting the loop header to its exit to avoid the PDT update issue before. It might make sense to briefly explain why first deleting and then adding a new edge will break.

In any case, these are at most minor issues. Feel free to commit.

Best,
Tobias



================
Comment at: include/llvm/Support/GenericDomTree.h:492
+  /// the same time, as the incremental algorithm doesn't walk the tree above
+  /// the NearestCommonDominator of a deleted edge
   ///
----------------
That looks good to me.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:248
+  // Connect the preheader to the exit block. Keep the old edge to the header
+  // around to perform dominator tree update in two separate steps -- insertion
+  // of the edge preheader->exit and deletion of the edge preheader->header.
----------------
THE dominator tree update


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:260
+  // Remove the old branch. The conditional branch becomes a new terminator.
+  OldBr->eraseFromParent();
 
----------------
Could you maybe move the "  DT.insertEdge(Preheader, ExitBlock);" this makes clear that the DT update comes right after the CFG change. Now it is hidden after the PHI update.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:297
 
-    // Remove the block from the reference counting scheme, so that we can
-    // delete it freely later.
-    (*LI)->dropAllReferences();
-  }
+  // Inform the domiantor tree about the removed edge.
+  DT.deleteEdge(Preheader, L->getHeader());
----------------
dominator


================
Comment at: unittests/IR/DominatorTreeTest.cpp:538
+
+  // It is possible to perform a multiple deletions and
+  // inform the DominatorTree about them of them at the same time, if the all of
----------------
Remove "a"


================
Comment at: unittests/IR/DominatorTreeTest.cpp:539
+  // It is possible to perform a multiple deletions and
+  // inform the DominatorTree about them of them at the same time, if the all of
+  // the deletions happen in different subtrees.
----------------
Remove "the"
Remove "of them"


https://reviews.llvm.org/D35391