[Attributor] Fold terminators before changing instructions to unreachable

[baziotis]

Summary

It is possible that an instruction to be changed to unreachable is in the same block with a terminator that can be constant-folded.
In this case, as of now, the instruction will be changed to unreachable before the terminator is folded. But, then the whole BB becomes
invalidated and so when we go ahead to fold the terminator, we trap.

Change the order of these two.

[jdoerfert]

This makes sense. However, this is not NFC. Do you have a test case?

[baziotis]

Example:

define internal i32 @callee(i1 %C, i32* %A) {
entry:
  %A.0 = load i32, i32* null
  br i1 %C, label %T, label %F

T:
  ret i32 %A.0

F:
  ret i32 1
}

define i32 @foo() {
  %X = call i32 @callee(i1 false, i32* null)
  ret i32 %X
}

I'll come back tomorrow to add it as a test.

[baziotis]

In D75780#1910596, @jdoerfert wrote:

This makes sense. However, this is not NFC. Do you have a test case?

My bad, let me actually add the test case now.

[baziotis]

Added test case.

[jdoerfert]

LGTM

llvm/lib/Transforms/IPO/Attributor.cpp

for (auto &V : InvokeWithDeadSuccessor)
             ToBeChangedToUnreachableInsts.insert(NormalNextIP);
         } else {
           assert(NormalBBIsDead && "Broken invariant!");
           if (!NormalDestBB->getUniquePredecessor())
             NormalDestBB = SplitBlockPredecessors(NormalDestBB, {BB}, ".dead");
           ToBeChangedToUnreachableInsts.insert(&NormalDestBB->front());
         }
       }
+    for (Instruction *I : TerminatorsToFold) {
+      CGModifiedFunctions.insert(I->getFunction());
+      ConstantFoldTerminator(I->getParent());
+    }
     for (auto &V : ToBeChangedToUnreachableInsts)
       if (Instruction *I = dyn_cast_or_null<Instruction>(V)) {
         CGModifiedFunctions.insert(I->getFunction());
         changeToUnreachable(I, /* UseLLVMTrap */ false);
       }
-    for (Instruction *I : TerminatorsToFold) {
-      CGModifiedFunctions.insert(I->getFunction());
-      ConstantFoldTerminator(I->getParent());
-    }
 
     for (auto &V : ToBeDeletedInsts) {
       if (Instruction *I = dyn_cast_or_null<Instruction>(V)) {
         CGModifiedFunctions.insert(I->getFunction());
         if (!I->getType()->isVoidTy())
           I->replaceAllUsesWith(UndefValue::get(I->getType()));
         if (!isa<PHINode>(I) && isInstructionTriviallyDead(I))
           DeadInsts.push_back(I);

llvm/test/Transforms/Attributor/undefined_behavior.ll

; ATTRIBUTOR-NEXT:    ret i32 2
   %alloc = alloca i1
   %cond = load i1, i1* %alloc
   br i1 %cond, label %t, label %e
 t:
   ret i32 1
 e:
   ret i32 2
 }
+
+; Note that the `load` has UB (so it will be changed to unreachable)
+; and the branch is a terminator that can be constant-folded.
+; We want to test that doing both won't cause a segfault.
+define internal i32 @callee(i1 %C, i32* %A) {
+; ATTRIBUTOR-NOT: @callee(
+;
+entry:
+  %A.0 = load i32, i32* null
+  br i1 %C, label %T, label %F
+
+T:
+  ret i32 %A.0
+
+F:
+  ret i32 1
+}
+
+define i32 @foo() {
+; ATTRIBUTOR-LABEL: @foo()
+; ATTRIBUTOR-NEXT:    ret i32 1
+  %X = call i32 @callee(i1 false, i32* null)
+  ret i32 %X
+}