[SimplifyCFG] Use applyUpdatesPermissive in HoistThenElseCodeToIf.

[fhahn]

Summary

The updates can in theory contain duplicates if the branch has the same
true and false successors. This is not really an issue when used in
SimplifyCFG, because such branches are simplified first. But it could
happen if it is used as a utility.

[lebedev.ri]

Please give me the testcase.

[fhahn]

In D101289#2716447, @lebedev.ri wrote:

Please give me the testcase.

I don't think it's possible to have a test case that's just using SimplifyCFG, because the branch would be simplified first. But there should be a crashing one in llvm/test/Transforms/LoopVectorize/AArch64/prepare-hoist-sink.ll from D101290

[lebedev.ri]

How about this then?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1620–1628

1632

[fhahn]

Avoid using applyUpdatesPermissive, by directly handling the case with duplicated successors.

[fhahn]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1620–1628

Done! I think we also need handling for the Insert part, because BB1 could also have duplicated successors.

[lebedev.ri]

LG with nits, thanks.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1613

You don't need this to be a vector.

1620–1628

Hmm. Only if you can add a test that will crash otherwise :)

1621–1622

[fhahn]

@lebedev.ri should we push this, even though we did not go with D101290 ?

[lebedev.ri]

Uh, i think i going to go with: we need a test first.

[fhahn]

There are no plans to use this as utility which could trigger the crash mentioned above. Abandoning for now.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

for (PHINode &PN : Succ->phis()) {
       for (unsigned i = 0, e = PN.getNumIncomingValues(); i != e; ++i)
         if (PN.getIncomingBlock(i) == BB1 || PN.getIncomingBlock(i) == BB2)
           PN.setIncomingValue(i, SI);
     }
   }
 
   SmallVector<DominatorTree::UpdateType, 4> Updates;
 
+  SmallSetVector<BasicBlock *, 4> SuccsToUpdate;
   // Update any PHI nodes in our new successors.
   for (BasicBlock *Succ : successors(BB1)) {
     AddPredecessorToBlock(Succ, BIParent, BB1);
     if (DTU)
-      Updates.push_back({DominatorTree::Insert, BIParent, Succ});
+      SuccsToUpdate.insert(Succ);
   }
 
-  if (DTU)
-    for (BasicBlock *Succ : successors(BI))
-      Updates.push_back({DominatorTree::Delete, BIParent, Succ});
+  if (DTU) {
+    for (BasicBlock *Succ : SuccsToUpdate)
+      Updates.push_back({DominatorTree::Insert, BIParent, Succ});
+
+    Updates.push_back({DominatorTree::Delete, BIParent, BB1});
+    if (BB1 != BB2)
+      Updates.push_back({DominatorTree::Delete, BIParent, BB2});
+  }
 
   EraseTerminatorAndDCECond(BI);
   if (DTU)
     DTU->applyUpdates(Updates);
   return Changed;
 }
 
 // Check lifetime markers.
 static bool isLifeTimeMarker(const Instruction *I) {
   if (auto II = dyn_cast<IntrinsicInst>(I)) {
     switch (II->getIntrinsicID()) {
     default: