[SimplifyCFG] Use make_early_inc_range() while deleting instructions

[aheejin]

Summary

We are deleting phi nodes within the for loop, so this makes sure we
increment the iterator before we delete the instruction pointed by the
iterator.

This started to break in
https://github.com/llvm/llvm-project/commit/a0be08164622bf938855ff5d19dd8e9d0c96b9b3.

[dschuff]

Looks straightforward, thanks for tracking this down.

[dschuff]

nice small reproducer too.

[lebedev.ri]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

4503

Use make_early_inc_range()?

[aheejin]

Use make_early_inc_range()

[lebedev.ri]

LG, thank you, and sorry for the trouble!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

4506

Is llvm:: really needed?

[aheejin]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

4506

No it isn't. Thanks; removed.

[aheejin]

Remove unnecessary llvm::

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

for (PHINode &DestPN : UnwindDest->phis()) {
         Value *Incoming =
             NeedPHITranslation ? SrcPN->getIncomingValueForBlock(Pred) : SrcVal;
         DestPN.addIncoming(Incoming, Pred);
       }
     }
 
     // Sink any remaining PHI nodes directly into UnwindDest.
     Instruction *InsertPt = DestEHPad;
-    for (PHINode &PN : BB->phis()) {
+    for (auto I = BB->begin(), IE = BB->getFirstNonPHI()->getIterator();
+         I != IE;) {
       // The iterator must be incremented here because the instructions are
       // being moved to another block.
-      if (PN.use_empty() || !PN.isUsedOutsideOfBlock(BB))
+      PHINode *PN = cast<PHINode>(I++);
+      if (PN->use_empty() || !PN->isUsedOutsideOfBlock(BB))
         // If the PHI node has no uses or all of its uses are in this basic
         // block (meaning they are debug or lifetime intrinsics), just leave
         // it.  It will be erased when we erase BB below.
         continue;
 
       // Otherwise, sink this PHI node into UnwindDest.
       // Any predecessors to UnwindDest which are not already represented
       // must be back edges which inherit the value from the path through
       // BB.  In this case, the PHI value must reference itself.
       for (auto *pred : predecessors(UnwindDest))
         if (pred != BB)
-          PN.addIncoming(&PN, pred);
-      PN.moveBefore(InsertPt);
+          PN->addIncoming(PN, pred);
+      PN->moveBefore(InsertPt);
       // Also, add a dummy incoming value for the original BB itself,
       // so that the PHI is well-formed until we drop said predecessor.
-      PN.addIncoming(UndefValue::get(PN.getType()), BB);
+      PN->addIncoming(UndefValue::get(PN->getType()), BB);
     }
   }
 
   std::vector<DominatorTree::UpdateType> Updates;
 
   // We use make_early_inc_range here because we will remove all predecessors.
   for (BasicBlock *PredBB : llvm::make_early_inc_range(predecessors(BB))) {
     if (UnwindDest == nullptr) {

llvm/test/Transforms/SimplifyCFG/cleanup-phis.ll

+; RUN: opt < %s -simplifycfg -S | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+; This is a regression test for a bug in which we used phis() in a for loop
+; while deleting phi nodes.
+
+define void @cleanup_phis() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+bb0:
+  invoke void @foo()
+          to label %bb1 unwind label %ehcleanup
+
+bb1:                                              ; preds = %bb0
+  invoke void @foo()
+          to label %invoke.cont unwind label %ehcleanup
+
+invoke.cont:                                      ; preds = %bb1
+  ret void
+
+ehcleanup:                                       ; preds = %bb1, %bb0
+  %phi0 = phi i32 [ 0, %bb0 ], [ 1, %bb1 ]
+  %phi1 = phi i32 [ 2, %bb0 ], [ 3, %bb1 ]
+  %0 = cleanuppad within none []
+  cleanupret from %0 unwind label %catchswitch
+
+; These two phi nodes were originally in ehcleanup. Both phi nodes should be
+; correctly copied to this catchswitch BB.
+; CHECK: catchswitch:
+; CHECK-NEXT:  %phi0 = phi i32 [ 0, %bb0 ], [ 1, %bb1 ]
+; CHECK-NEXT:  %phi1 = phi i32 [ 2, %bb0 ], [ 3, %bb1 ]
+catchswitch:                                      ; preds = %ehcleanup
+  %1 = catchswitch within none [label %catch] unwind to caller
+
+catch:                                            ; preds = %catchswitch
+  %2 = catchpad within %1 [i8* null]
+  call void @bar(i32 %phi0, i32 %phi1)
+  unreachable
+}
+
+declare void @foo()
+declare void @bar(i32, i32)
+declare i32 @__gxx_wasm_personality_v0(...)