[SimpleLoopUnswitch] Fix DT updates for trivial branch unswitching.

[asbirlea]

Summary

Fixing 2 issues with the DT update in trivial branch switching, though I don't have a case where DT update fails.

1. After splitting ParentBB->UnswitchedBB edge, new edges become: ParentBB->LoopExitBB->UnswitchedBB, so remove ParentBB->LoopExitBB edge.
2. AFAIU, for multiple CFG changes, DT should be updated using batch updates, vs consecutive addEdge and removeEdge calls.

[kuhar]

This case requires batch updater. It's always surprising that these kinds of bugs can stay undetected for months.

[chandlerc]

LGTM, but *please* update the comments in the update API to make this clear. I specifically thought there was no difference here after reading the comments, so I think they need to make this way more clear.

[kuhar]

@chandlerc FYI, all existing uses of the 'raw' api (DT.insert/deleteEdge, DT.applyUpdates) are going to be replaced with the new unified API after branching to 8.0. @NutshellySima is working on this.
The plan is also to hide insert/deleteEdge completely, so that we won't have bugs like this in the future.

[chandlerc]

In D49925#1178887, @kuhar wrote:

@chandlerc FYI, all existing uses of the 'raw' api (DT.insert/deleteEdge, DT.applyUpdates) are going to be replaced with the new unified API after branching to 8.0. @NutshellySima is working on this.
The plan is also to hide insert/deleteEdge completely, so that we won't have bugs like this in the future.

Sure, but a *bunch* of code is going to use these APIs before that point, so I think we should still invest in their documentation.

While the comments on insertEdge and deleteEdge try to make the correctness issues clear, if you don't want folks to use them, they could be much more clear about that.

The comment on applyUpdates that says it should be faster than calling insertEdge and deleteEdge directly is the one that confused me -- it makes it sound like this is just a performance tradeoff. Could improve it just by reminding the reader that there is a bigger semantic difference. Something like "In addition to supporting a batch of updates (which direct calls don't), this is more efficient than directly applying the updates ...." or something.

[kuhar]

@chandlerc OK, now I finally understand why it's possible to get confused here. I put a patch with modified documentation here: D49944.

llvm/trunk/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT,
   // Rewrite the relevant PHI nodes.
   if (UnswitchedBB == LoopExitBB)
     rewritePHINodesForUnswitchedExitBlock(*UnswitchedBB, *ParentBB, *OldPH);
   else
     rewritePHINodesForExitAndUnswitchedBlocks(*LoopExitBB, *UnswitchedBB,
                                               *ParentBB, *OldPH, FullUnswitch);
 
   // Now we need to update the dominator tree.
-  DT.insertEdge(OldPH, UnswitchedBB);
+  SmallVector<DominatorTree::UpdateType, 2> DTUpdates;
+  DTUpdates.push_back({DT.Insert, OldPH, UnswitchedBB});
   if (FullUnswitch)
-    DT.deleteEdge(ParentBB, UnswitchedBB);
+    DTUpdates.push_back({DT.Delete, ParentBB, LoopExitBB});
+  DT.applyUpdates(DTUpdates);
 
   // The constant we can replace all of our invariants with inside the loop
   // body. If any of the invariants have a value other than this the loop won't
   // be entered.
   ConstantInt *Replacement = ExitDirection
                                  ? ConstantInt::getFalse(BI.getContext())
                                  : ConstantInt::getTrue(BI.getContext());