[PATCH] D35528: [Dominators] Teach LoopUnswitch to use the incremental API

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I'm a little fried at the moment, so just some drive-by comments. I wonder if we can make the memory management automatic instead of calling naked delete.



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:938
   // insert the new conditional branch.
+  assert(isa<BranchInst>(loopPreheader->getTerminator()));
+  auto *OldBranch = cast<BranchInst>(loopPreheader->getTerminator());
----------------
message in the assertion?


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:939
+  assert(isa<BranchInst>(loopPreheader->getTerminator()));
+  auto *OldBranch = cast<BranchInst>(loopPreheader->getTerminator());
   EmitPreheaderBranchOnCondition(Cond, Val, NewExit, NewPH,
----------------
instead of using isa<> + cast<> can you use `dyn_cast<>` maybe ?


================
Comment at: lib/Transforms/Utils/BreakCriticalEdges.cpp:224-227
   if (DT) {
-    DomTreeNode *TINode = DT->getNode(TIBB);
-
-    // The new block is not the immediate dominator for any other nodes, but
-    // TINode is the immediate dominator for the new node.
-    //
-    if (TINode) {       // Don't break unreachable code!
-      DomTreeNode *NewBBNode = DT->addNewBlock(NewBB, TIBB);
-      DomTreeNode *DestBBNode = nullptr;
-
-      // If NewBBDominatesDestBB hasn't been computed yet, do so with DT.
-      if (!OtherPreds.empty()) {
-        DestBBNode = DT->getNode(DestBB);
-        while (!OtherPreds.empty() && NewBBDominatesDestBB) {
-          if (DomTreeNode *OPNode = DT->getNode(OtherPreds.back()))
-            NewBBDominatesDestBB = DT->dominates(DestBBNode, OPNode);
-          OtherPreds.pop_back();
-        }
-        OtherPreds.clear();
-      }
-
-      // If NewBBDominatesDestBB, then NewBB dominates DestBB, otherwise it
-      // doesn't dominate anything.
-      if (NewBBDominatesDestBB) {
-        if (!DestBBNode) DestBBNode = DT->getNode(DestBB);
-        DT->changeImmediateDominator(DestBBNode, NewBBNode);
-      }
-    }
+    DT->insertEdge(TIBB, NewBB);
+    DT->insertEdge(NewBB, DestBB);
+    DT->deleteEdge(TIBB, DestBB);
----------------
Can you add a more descriptive comment here?


https://reviews.llvm.org/D35528