[Attributor][FIX] Carefully change invokes to calls (after manifest)

Before we manually inserted unreachable early but that could lead to
broken PHI nodes. Now we use the existing late modification
functionality.

llvm/include/llvm/Transforms/IPO/Attributor.h

@@ -865,6 +865,13 @@ struct Attributor {
     ToBeChangedToUnreachableInsts.insert(I);
   }
 
+  /// Record that \p II has at least one dead successor block. This information
+  /// is used, e.g., to replace \p II with a call, after information was
+  /// manifested.
+  void registerInvokeWithDeadSuccessor(InvokeInst &II) {
+    InvokeWithDeadSuccessor.push_back(&II);
+  }
+
   /// Record that \p I is deleted after information was manifested. This also
   /// triggers deletion of trivially dead istructions.
   void deleteAfterManifest(Instruction &I) { ToBeDeletedInsts.insert(&I); }
@@ -1176,6 +1183,9 @@ private:
   /// Instructions we replace with `unreachable` insts after manifest is done.
   SmallDenseSet<WeakVH, 16> ToBeChangedToUnreachableInsts;
 
+  /// Invoke instructions with at least a single dead successor block.
+  SmallVector<WeakVH, 16> InvokeWithDeadSuccessor;
+
   /// Functions, blocks, and instructions we delete after manifest is done.
   ///
   ///{

llvm/lib/Transforms/IPO/Attributor.cpp

@@ -31,6 +31,7 @@
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
@@ -2824,90 +2825,12 @@ struct AAIsDeadFunction : public AAIsDead {
       bool MayReturn = !NoReturnAA.isAssumedNoReturn();
       if (MayReturn && (!Invoke2CallAllowed || !isa<InvokeInst>(CB)))
         continue;
-      Instruction *I = const_cast<Instruction *>(DeadEndI);
-      BasicBlock *BB = I->getParent();
-      Instruction *SplitPos = I->getNextNode();
-      // TODO: mark stuff before unreachable instructions as dead.
 
-      if (auto *II = dyn_cast<InvokeInst>(I)) {
+      if (auto *II = dyn_cast<InvokeInst>(DeadEndI))
-        // If we keep the invoke the split position is at the beginning of the
+        A.registerInvokeWithDeadSuccessor(const_cast<InvokeInst &>(*II));
-        // normal desitination block (it invokes a noreturn function after all).
+      else
-        BasicBlock *NormalDestBB = II->getNormalDest();
+        A.changeToUnreachableAfterManifest(
-        SplitPos = &NormalDestBB->front();
+            const_cast<Instruction *>(DeadEndI->getNextNode()));
-
-        /// Invoke is replaced with a call and unreachable is placed after it if
-        /// the callee is nounwind and noreturn. Otherwise, we keep the invoke
-        /// and only place an unreachable in the normal successor.
-        if (Invoke2CallAllowed) {
-          if (II->getCalledFunction()) {
-            const IRPosition &IPos = IRPosition::callsite_function(*II);
-            const auto &AANoUnw = A.getAAFor<AANoUnwind>(*this, IPos);
-            if (AANoUnw.isAssumedNoUnwind()) {
-              LLVM_DEBUG(dbgs()
-                         << "[AAIsDead] Replace invoke with call inst\n");
-              CallInst *CI = createCallMatchingInvoke(II);
-              CI->insertBefore(II);
-              CI->takeName(II);
-              replaceAllInstructionUsesWith(*II, *CI);
-
-              // If this is a nounwind + mayreturn invoke we only remove the
-              // unwind edge. This is done by moving the invoke into a new and
-              // dead block and connecting the normal destination of the invoke
-              // with a branch that follows the call replacement we created
-              // above.
-              if (MayReturn) {
-                BasicBlock *NewDeadBB =
-                    SplitBlock(BB, II, nullptr, nullptr, nullptr, ".i2c");
-                assert(isa<BranchInst>(BB->getTerminator()) &&
-                       BB->getTerminator()->getNumSuccessors() == 1 &&
-                       BB->getTerminator()->getSuccessor(0) == NewDeadBB);
-                new UnreachableInst(I->getContext(), NewDeadBB);
-                BB->getTerminator()->setOperand(0, NormalDestBB);
-                A.deleteAfterManifest(*II);
-                continue;
-              }
-
-              // We do not need an invoke (II) but instead want a call followed
-              // by an unreachable. However, we do not remove II as other
-              // abstract attributes might have it cached as part of their
-              // results. Given that we modify the CFG anyway, we simply keep II
-              // around but in a new dead block. To avoid II being live through
-              // a different edge we have to ensure the block we place it in is
-              // only reached from the current block of II and then not reached
-              // at all when we insert the unreachable.
-              SplitBlockPredecessors(NormalDestBB, {BB}, ".i2c");
-              SplitPos = CI->getNextNode();
-            }
-          }
-        }
-
-        if (SplitPos == &NormalDestBB->front()) {
-          // If this is an invoke of a noreturn function the edge to the normal
-          // destination block is dead but not necessarily the block itself.
-          // TODO: We need to move to an edge based system during deduction and
-          //       also manifest.
-          assert(!NormalDestBB->isLandingPad() &&
-                 "Expected the normal destination not to be a landingpad!");
-          if (NormalDestBB->getUniquePredecessor() == BB) {
-            assumeLive(A, *NormalDestBB);
-          } else {
-            BasicBlock *SplitBB =
-                SplitBlockPredecessors(NormalDestBB, {BB}, ".dead");
-            // The split block is live even if it contains only an unreachable
-            // instruction at the end.
-            assumeLive(A, *SplitBB);
-            SplitPos = SplitBB->getTerminator();
-            HasChanged = ChangeStatus::CHANGED;
-          }
-        }
-      }
-
-      if (isa_and_nonnull<UnreachableInst>(SplitPos))
-        continue;
-
-      BB = SplitPos->getParent();
-      SplitBlock(BB, SplitPos);
-      A.changeToUnreachableAfterManifest(BB->getTerminator());
       HasChanged = ChangeStatus::CHANGED;
     }
 
@@ -5668,6 +5591,32 @@ ChangeStatus Attributor::run(Module &M) {
         }
       }
     }
+    for (auto &V : InvokeWithDeadSuccessor)
+      if (InvokeInst *II = dyn_cast_or_null<InvokeInst>(V)) {
+        bool UnwindBBIsDead = II->hasFnAttr(Attribute::NoUnwind);
+        bool NormalBBIsDead = II->hasFnAttr(Attribute::NoReturn);
+        bool Invoke2CallAllowed =
+            !AAIsDeadFunction::mayCatchAsynchronousExceptions(
+                *II->getFunction());
+        assert((UnwindBBIsDead || NormalBBIsDead) &&
+               "Invoke does not have dead successors!");
+        BasicBlock *BB = II->getParent();
+        BasicBlock *NormalDestBB = II->getNormalDest();
+        if (UnwindBBIsDead) {
+          Instruction *NormalNextIP = &NormalDestBB->front();
+          if (Invoke2CallAllowed) {
+            changeToCall(II);
+            NormalNextIP = BB->getTerminator();
+          }
+          if (NormalBBIsDead)
+            ToBeChangedToUnreachableInsts.insert(NormalNextIP);
+        } else {
+          assert(NormalBBIsDead && "Broken invariant!");
+          if (!NormalDestBB->getUniquePredecessor())
+            NormalDestBB = SplitBlockPredecessors(NormalDestBB, {BB}, ".dead");
+          ToBeChangedToUnreachableInsts.insert(&NormalDestBB->front());
+        }
+      }
     for (auto &V : ToBeChangedToUnreachableInsts)
       if (Instruction *I = dyn_cast_or_null<Instruction>(V))
         changeToUnreachable(I, /* UseLLVMTrap */ false);
@@ -6337,7 +6286,9 @@ static bool runAttributorOnModule(Module &M, AnalysisGetter &AG) {
     A.identifyDefaultAbstractAttributes(F);
   }
 
-  return A.run(M) == ChangeStatus::CHANGED;
+  bool Changed = A.run(M) == ChangeStatus::CHANGED;
+  assert(!verifyModule(M, &errs()) && "Module verification failed!");
+  return Changed;
 }
 
 PreservedAnalyses AttributorPass::run(Module &M, ModuleAnalysisManager &AM) {

llvm/test/Transforms/Attributor/ArgumentPromotion/crash.ll

@@ -11,10 +11,6 @@ define void @zot() personality i32 (...)* @wibble {
 ; ATTRIBUTOR-NEXT:  bb:
 ; ATTRIBUTOR-NEXT:    call void @hoge()
 ; ATTRIBUTOR-NEXT:    unreachable
-; ATTRIBUTOR:       bb.split:
-; ATTRIBUTOR-NEXT:    unreachable
-; ATTRIBUTOR:       bb1.i2c:
-; ATTRIBUTOR-NEXT:    unreachable
 ; ATTRIBUTOR:       bb1:
 ; ATTRIBUTOR-NEXT:    unreachable
 ; ATTRIBUTOR:       bb2:
@@ -47,8 +43,6 @@ define internal void @hoge() {
 ; ATTRIBUTOR-LABEL: define {{[^@]+}}@hoge()
 ; ATTRIBUTOR-NEXT:  bb:
 ; ATTRIBUTOR-NEXT:    unreachable
-; ATTRIBUTOR:       bb.split:
-; ATTRIBUTOR-NEXT:    unreachable
 ;
 bb:
   %tmp = call fastcc i8* @spam(i1 (i8*)* @eggs)
@@ -77,8 +71,6 @@ define i32 @test_inf_promote_caller(i32 %arg) {
 ; CHECK-SAME: (i32 [[ARG:%.*]])
 ; CHECK-NEXT:  bb:
 ; CHECK-NEXT:    unreachable
-; CHECK:       bb.split:
-; CHECK-NEXT:    unreachable
 ;
 bb:
   %tmp = alloca %S

llvm/test/Transforms/Attributor/ArgumentPromotion/fp80.ll

@@ -17,8 +17,6 @@ define void @run() {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @CaptureAStruct(%struct.Foo* nofree nonnull readonly align 8 dereferenceable(16) @a)
 ; CHECK-NEXT:    unreachable
-; CHECK:       entry.split:
-; CHECK-NEXT:    unreachable
 ;
 entry:
   tail call i8 @UseLongDoubleUnsafely(%union.u* byval align 16 bitcast (%struct.s* @b to %union.u*))

llvm/test/Transforms/Attributor/ArgumentPromotion/live_called_from_dead.ll

@@ -1,4 +1,4 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
 ; RUN: opt -S -basicaa -attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s --check-prefixes=CHECK,OLDPM_MODULE
 ; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s --check-prefixes=CHECK,NEWPM_MODULE
 

llvm/test/Transforms/Attributor/ArgumentPromotion/nonzero-address-spaces.ll

@@ -13,8 +13,6 @@ define i32 @bar() {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CALL:%.*]] = call addrspace(1) i32 @foo()
 ; CHECK-NEXT:    unreachable
-; CHECK:       entry.split:
-; CHECK-NEXT:    unreachable
 ;
 
 entry:

llvm/test/Transforms/Attributor/IPConstantProp/recursion.ll

@@ -12,8 +12,6 @@ define internal i32 @foo(i32 %X) {
 define void @bar() {
 ; CHECK-LABEL: define {{[^@]+}}@bar()
 ; CHECK-NEXT:    unreachable
-; CHECK:       .split:
-; CHECK-NEXT:    unreachable
 ;
   call i32 @foo( i32 17 )         ; <i32>:1 [#uses=0]
   ret void

llvm/test/Transforms/Attributor/IPConstantProp/return-constant.ll

@@ -8,8 +8,6 @@ define i1 @invokecaller(i1 %C) personality i32 (...)* @__gxx_personality_v0 {
 ; CHECK-SAME: (i1 [[C:%.*]]) #0 personality i32 (...)* @__gxx_personality_v0
 ; CHECK-NEXT:    [[X:%.*]] = call i32 @foo(i1 [[C]])
 ; CHECK-NEXT:    br label [[OK:%.*]]
-; CHECK:       .i2c:
-; CHECK-NEXT:    unreachable
 ; CHECK:       OK:
 ; CHECK-NEXT:    [[Y:%.*]] = icmp ne i32 52, 0
 ; CHECK-NEXT:    ret i1 [[Y]]

llvm/test/Transforms/Attributor/liveness.ll

@@ -277,6 +277,96 @@ cleanup:
   ret i32 0
 }
 
+; UTC_ARGS: --turn on
+
+; TEST 5.4 unounwind invoke instruction replaced by a call and a branch instruction put after it.
+define i32 @invoke_nounwind_phi(i32 %a) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+; CHECK-LABEL: define {{[^@]+}}@invoke_nounwind_phi
+; CHECK-SAME: (i32 [[A:%.*]]) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[A]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK:       cond.true:
+; CHECK-NEXT:    call void @normal_call()
+; CHECK-NEXT:    [[CALL:%.*]] = call i32 @foo_nounwind()
+; CHECK-NEXT:    br label [[CONTINUE:%.*]]
+; CHECK:       cond.false:
+; CHECK-NEXT:    call void @normal_call()
+; CHECK-NEXT:    [[CALL1:%.*]] = call i32 @bar()
+; CHECK-NEXT:    br label [[CONTINUE]]
+; CHECK:       continue:
+; CHECK-NEXT:    [[P:%.*]] = phi i32 [ 0, [[COND_TRUE]] ], [ 1, [[COND_FALSE]] ]
+; CHECK-NEXT:    ret i32 [[P]]
+; CHECK:       cleanup:
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %cmp = icmp eq i32 %a, 0
+  br i1 %cmp, label %cond.true, label %cond.false
+
+cond.true:                                        ; preds = %entry
+  call void @normal_call()
+  %call = invoke i32 @foo_nounwind() to label %continue
+  unwind label %cleanup
+
+cond.false:                                       ; preds = %entry
+  call void @normal_call()
+  %call1 = call i32 @bar()
+  br label %continue
+
+continue:
+  %p = phi i32 [ 0, %cond.true ], [ 1, %cond.false ]
+  ret i32 %p
+
+cleanup:
+  %res = landingpad { i8*, i32 } catch i8* null
+  ret i32 0
+}
+
+; TEST 5.5 unounwind invoke instruction replaced by a call and a branch instruction put after it.
+define i32 @invoke_nounwind_phi_dom(i32 %a) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+; CHECK-LABEL: define {{[^@]+}}@invoke_nounwind_phi_dom
+; CHECK-SAME: (i32 [[A:%.*]]) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[A]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK:       cond.true:
+; CHECK-NEXT:    call void @normal_call()
+; CHECK-NEXT:    [[CALL:%.*]] = call i32 @foo_nounwind()
+; CHECK-NEXT:    br label [[CONTINUE:%.*]]
+; CHECK:       cond.false:
+; CHECK-NEXT:    call void @normal_call()
+; CHECK-NEXT:    [[CALL1:%.*]] = call i32 @bar()
+; CHECK-NEXT:    br label [[CONTINUE]]
+; CHECK:       continue:
+; CHECK-NEXT:    [[P:%.*]] = phi i32 [ [[CALL]], [[COND_TRUE]] ], [ [[CALL1]], [[COND_FALSE]] ]
+; CHECK-NEXT:    ret i32 [[P]]
+; CHECK:       cleanup:
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %cmp = icmp eq i32 %a, 0
+  br i1 %cmp, label %cond.true, label %cond.false
+
+cond.true:                                        ; preds = %entry
+  call void @normal_call()
+  %call = invoke i32 @foo_nounwind() to label %continue
+  unwind label %cleanup
+
+cond.false:                                       ; preds = %entry
+  call void @normal_call()
+  %call1 = call i32 @bar()
+  br label %continue
+
+continue:
+  %p = phi i32 [ %call, %cond.true ], [ %call1, %cond.false ]
+  ret i32 %p
+
+cleanup:
+  %res = landingpad { i8*, i32 } catch i8* null
+  ret i32 0
+}
+
 ; UTC_ARGS: --turn off
 
 ; TEST 6: Undefined behvior, taken from LangRef.
@@ -707,7 +797,6 @@ define internal void @dead_e2() { ret void }
 ; CHECK-NEXT: define internal void @non_dead_d15()
 ; CHECK-NOT: define internal void @dead_e
 
-
 declare void @blowup() noreturn
 define void @live_with_dead_entry() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 ; CHECK:      define void @live_with_dead_entry(
@@ -735,19 +824,19 @@ define void @live_with_dead_entry_lp() personality i8* bitcast (i32 (...)* @__gx
 ; CHECK:      define void @live_with_dead_entry_lp(
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   invoke void @blowup()
-; CHECK-NEXT:           to label %live_with_dead_entry.dead unwind label %lp1
+; CHECK-NEXT:    to label %[[LIVE_WITH_DEAD_ENTRY_DEAD1:.*]] unwind label %[[LP1:.*]]
-; CHECK:      lp1:                                              ; preds = %entry
+; CHECK:      [[LP1]]:                                              ; preds = %entry
 ; CHECK-NEXT:   %lp = landingpad { i8*, i32 }
 ; CHECK-NEXT:           catch i8* null
 ; CHECK-NEXT:   invoke void @blowup()
-; CHECK-NEXT:           to label %live_with_dead_entry.dead1 unwind label %lp2
+; CHECK-NEXT:    to label %[[LIVE_WITH_DEAD_ENTRY_DEAD2:.*]] unwind label %[[LP2:.*]]
-; CHECK:      lp2:                                              ; preds = %lp1
+; CHECK:      [[LP2]]:                                              ; preds = %lp1
 ; CHECK-NEXT:   %0 = landingpad { i8*, i32 }
 ; CHECK-NEXT:           catch i8* null
 ; CHECK-NEXT:   br label %live_with_dead_entry
-; CHECK:      live_with_dead_entry.dead:
+; CHECK:      [[LIVE_WITH_DEAD_ENTRY_DEAD1]]:
 ; CHECK-NEXT:   unreachable
-; CHECK:      live_with_dead_entry.dead1:
+; CHECK:      [[LIVE_WITH_DEAD_ENTRY_DEAD2]]:
 ; CHECK-NEXT:   unreachable
 ; CHECK:      live_with_dead_entry:                             ; preds = %lp2
 ; CHECK-NEXT:   ret void

llvm/test/Transforms/Attributor/noreturn_async.ll

@@ -100,9 +100,9 @@ define dso_local i32 @"?catchoverflow@@YAHXZ_may_throw"()  personality i8* bitca
 entry:
   %retval = alloca i32, align 4
   %__exception_code = alloca i32, align 4
-; CHECK: invoke void @"?overflow@@YAXXZ_may_throw"() 
+; CHECK: invoke void @"?overflow@@YAXXZ_may_throw"()
 ; CHECK:          to label %invoke.cont unwind label %catch.dispatch
-  invoke void @"?overflow@@YAXXZ_may_throw"() 
+  invoke void @"?overflow@@YAXXZ_may_throw"()
           to label %invoke.cont unwind label %catch.dispatch
 
 invoke.cont:                                      ; preds = %entry