[SimplifyCFG] Prohibit hoisting of llvm.deoptimize calls

[dmakogon]

Summary

This prohibits hoisiting identical llvm.deoptimize calls from 'then' and 'else' blocks of a conditional branch.
This fixes a crash that happened because we didn't hoist the return instructions together with the llvm.deoptimize calls, so the verifier would crash.

[mkazantsev]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1521

To me it looks like overkill. As far as I understand the problem, the crash happens when we hoist the deoptimize call which is then not followed by return after this hoisting. Hoisting any other instruction is perfectly legal, and it also reduces code size. Let's make the fix as non-invasive as possible.

I still think that the requirement to have deoptimize immediately followed by return is useless and weird, but it's out of scope of this one. Until cancelled, we should sustain it.

[dmakogon]

Instead of prohibiting hoisting from deoptimize blocks, just disallow hoisiting of llvm.deoptimize calls

[mkazantsev]

That will do, please wait a day or two if anyone has concerns.

[fhahn]

LGTM, thanks! Would be good to simplify the bundle if possible.

llvm/test/Transforms/SimplifyCFG/dont-hoist-deoptimize.ll

31

Is this massive bundle needed or can it be reduced?

Same for the bundle below.

[dmakogon]

llvm/test/Transforms/SimplifyCFG/dont-hoist-deoptimize.ll

31

Yeah, they may be reduced. Thanks! Landed reduced version

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

if ((Flags & SkipSideEffect) &&
       (I->mayReadFromMemory() || I->mayHaveSideEffects()))
     return false;
 
   // Reordering across an instruction which does not necessarily transfer
   // control to the next instruction is speculation.
   if ((Flags & SkipImplicitControlFlow) && !isSafeToSpeculativelyExecute(I))
     return false;
 
+  // Hoisting of llvm.deoptimize is only legal together with the next return
+  // instruction, which this pass is not always able to do.
+  if (auto *CB = dyn_cast<CallBase>(I))
+    if (CB->getIntrinsicID() == Intrinsic::experimental_deoptimize)
+      return false;
+
   // It's also unsafe/illegal to hoist an instruction above its instruction
   // operands
   BasicBlock *BB = I->getParent();
   for (Value *Op : I->operands()) {
     if (auto *J = dyn_cast<Instruction>(Op))
       if (J->getParent() == BB)
         return false;
   }
bool SimplifyCFGOpt::HoistThenElseCodeToIf(BranchInst *BI,
   // by it's address.
   if (BB1->hasAddressTaken() || BB2->hasAddressTaken())
     return false;
 
   BasicBlock::iterator BB1_Itr = BB1->begin();
   BasicBlock::iterator BB2_Itr = BB2->begin();
 
   Instruction *I1 = &*BB1_Itr++, *I2 = &*BB2_Itr++;
   // Skip debug info if it is not identical.
   DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
   DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
   if (!DBI1 || !DBI2 || !DBI1->isIdenticalToWhenDefined(DBI2)) {
     while (isa<DbgInfoIntrinsic>(I1))
       I1 = &*BB1_Itr++;
     while (isa<DbgInfoIntrinsic>(I2))
       I2 = &*BB2_Itr++;
   }

llvm/test/Transforms/SimplifyCFG/dont-hoist-deoptimize.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S --passes='simplifycfg<hoist-common-insts>' -simplifycfg-hoist-common-skip-limit=0 %s | FileCheck %s
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @llvm.experimental.deoptimize.isVoid(...) #0
 
-; REQUIRES: asserts
-; XFAIL: *
-
 define void @widget(i1 %arg) {
+; CHECK-LABEL: @widget(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP:%.*]] = trunc i64 5 to i32
+; CHECK-NEXT:    br i1 [[ARG:%.*]], label [[BB1:%.*]], label [[BB4:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[TMP2:%.*]] = trunc i64 0 to i32
+; CHECK-NEXT:    [[TMP3:%.*]] = trunc i64 0 to i32
+; CHECK-NEXT:    call void (...) @llvm.experimental.deoptimize.isVoid(i32 13) #[[ATTR0:[0-9]+]] [ "deopt"() ]
+; CHECK-NEXT:    ret void
+; CHECK:       bb4:
+; CHECK-NEXT:    [[TMP6:%.*]] = trunc i64 1 to i32
+; CHECK-NEXT:    [[TMP7:%.*]] = trunc i64 0 to i32
+; CHECK-NEXT:    call void (...) @llvm.experimental.deoptimize.isVoid(i32 13) #[[ATTR0]] [ "deopt"() ]
+; CHECK-NEXT:    ret void
+;
 bb:
   br i1 %arg, label %bb1, label %bb4
 
 bb1:                                              ; preds = %bb
   %tmp = trunc i64 5 to i32
   %tmp2 = trunc i64 0 to i32
   %tmp3 = trunc i64 0 to i32
   call void (...) @llvm.experimental.deoptimize.isVoid(i32 13) #0 [ "deopt"() ]
   ret void
 
 bb4:                                              ; preds = %bb
   %tmp5 = trunc i64 5 to i32
   %tmp6 = trunc i64 1 to i32
   %tmp7 = trunc i64 0 to i32
   call void (...) @llvm.experimental.deoptimize.isVoid(i32 13) #0 [ "deopt"() ]
   ret void
 }
 
 attributes #0 = { nounwind }