[LoopUnswitch]Fix comparison for DomTree updates.

[asbirlea]

Summary

In LoopUnswitch when replacing a branch Parent -> Succ with a conditional
branch Parent -> True & Parent->False, the DomTree updates should insert an edge for
each of True/False if True/False are different than Succ, and delete Parent->Succ edge
if both are different. The comparison with Succ appears to be incorect,
it's comparing with Parent instead.
There is no test failing either before or after this change, but it seems to me this is
the right way to do the update.

[kuhar]

lib/Transforms/Scalar/LoopUnswitch.cpp

945 ↗

(On Diff #152387)

If it's illegal to loop back to preheader, maybe there should be an assert for that?

947 ↗

(On Diff #152387)

Is it possible that TrueDest == FalseDest? In this is the case, we should check it here as well.

[asbirlea]

lib/Transforms/Scalar/LoopUnswitch.cpp

945 ↗

(On Diff #152387)

I'm not sure we can get that info in this method. This is essentially an util, independent of the loopunswitch logic at the callsite.

947 ↗

(On Diff #152387)

This should not be possible.
The logic is contained to LoopUnswitch and the two callsites for this method each have a new block as the TrueDest and an existing block as FalseDest. I can add an assert (TrueDest != FalseDest), if you think it would be helpful.

[kuhar]

LGTM

lib/Transforms/Scalar/LoopUnswitch.cpp

947 ↗

(On Diff #152387)

I think it would make it clear for whoever tries to figure out the correct logic here later.

[asbirlea]

Add assert.

[asbirlea]

Thank you for the review!

lib/Transforms/Scalar/LoopUnswitch.cpp

947 ↗

(On Diff #152387)

Ack, done.

[chandlerc]

It'd be nice to get a test case here, but if its not feasible, the change is obviously correct (or at least an improvement!!!)

LGTM.

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

 /// otherwise branch to FalseDest. Insert the code immediately before OldBranch
 /// and remove (but not erase!) it from the function.
 void LoopUnswitch::EmitPreheaderBranchOnCondition(Value *LIC, Constant *Val,
                                                   BasicBlock *TrueDest,
                                                   BasicBlock *FalseDest,
                                                   BranchInst *OldBranch,
                                                   TerminatorInst *TI) {
   assert(OldBranch->isUnconditional() && "Preheader is not split correctly");
+  assert(TrueDest != FalseDest && "Branch targets should be different");
   // Insert a conditional branch on LIC to the two preheaders.  The original
   // code is the true version and the new code is the false version.
   Value *BranchVal = LIC;
   bool Swapped = false;
   if (!isa<ConstantInt>(Val) ||
       Val->getType() != Type::getInt1Ty(LIC->getContext()))
     BranchVal = new ICmpInst(OldBranch, ICmpInst::ICMP_EQ, LIC, Val);
   else if (Val != ConstantInt::getTrue(Val->getContext())) {
void LoopUnswitch::EmitPreheaderBranchOnCondition(Value *LIC, Constant *Val,
   // Remove the old branch so there is only one branch at the end. This is
   // needed to perform DomTree's internal DFS walk on the function's CFG.
   OldBranch->removeFromParent();
 
   // Inform the DT about the new branch.
   if (DT) {
     // First, add both successors.
     SmallVector<DominatorTree::UpdateType, 3> Updates;
-    if (TrueDest != OldBranchParent)
+    if (TrueDest != OldBranchSucc)
       Updates.push_back({DominatorTree::Insert, OldBranchParent, TrueDest});
-    if (FalseDest != OldBranchParent)
+    if (FalseDest != OldBranchSucc)
       Updates.push_back({DominatorTree::Insert, OldBranchParent, FalseDest});
     // If both of the new successors are different from the old one, inform the
     // DT that the edge was deleted.
     if (OldBranchSucc != TrueDest && OldBranchSucc != FalseDest) {
       Updates.push_back({DominatorTree::Delete, OldBranchParent, OldBranchSucc});
     }
 
     DT->applyUpdates(Updates);