[PM/LoopUnswitch] Add back a successor set that was removed based on

code review.

It turns out this *is* necessary, and I read the comment on the API
correctly the first time. ;]

The `applyUpdates` routine requires that updates are "balanced". This is
in order to cleanly handle cycles like inserting, removing, nad then
re-inserting the same edge. This precludes inserting the same edge
multiple times in a row as handling that would cause the insertion logic
to become *ordered* instead of *unordered* (which is what the API
provides).

It happens that in this specific case nothing (other than an assert and
contract violation) goes wrong because we're never inserting and
removing the same edge. The implementation *happens* to do the right
thing to eliminate redundant insertions in that case.

But the requirement is there and there is an assert to catch it.
Somehow, after the code review I never did another asserts-clang build
testing loop-unswich for a long time. As a consequence, I didn't notice
this despite a bunch of testing going on, but it shows up immediately
with an asserts build of clang itself.

llvm-svn: 331246

diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index 997e6ef..6a6900f 100644 (file)
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -883,9 +883,13 @@ static BasicBlock *buildClonedLoopBlocks(
             PN.removeIncomingValue(LoopBB, /*DeletePHIIfEmpty*/ false);
 
   // Record the domtree updates for the new blocks.
-  for (auto *ClonedBB : NewBlocks)
+  SmallPtrSet<BasicBlock *, 4> SuccSet;
+  for (auto *ClonedBB : NewBlocks) {
     for (auto *SuccBB : successors(ClonedBB))
-      DTUpdates.push_back({DominatorTree::Insert, ClonedBB, SuccBB});
+      if (SuccSet.insert(SuccBB).second)
+        DTUpdates.push_back({DominatorTree::Insert, ClonedBB, SuccBB});
+    SuccSet.clear();
+  }
 
   return ClonedPH;
 }