[SimplifyCFG] Skip dbg intrinsics when checking for branch-only BBs.

[fhahn]

Summary

Debug intrinsics are free to hoist and should be skipped when looking
for terminator-only blocks. As a consequence, we have to delegate to the
main hoisting loop to hoist any dbg intrinsics instead of jumping to the
terminator case directly.

This fixes PR49982.

[uabelho]

I've verified that this fixes the problem I saw. Thanks!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1442

Since I1NonDbg and I2NonDbg should be identical here I guess it doesn't really matter, but anyway: Wouldn't it make the patch look a little bit simpler if we used I1NonDbg here rather than changing from I1 to I2NonDbg?

[fhahn]

Update to use I1NonDbg instead of I2NonDbg, also clang-format.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1442

That's indeed better, thanks. Updated!

[lebedev.ri]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1442

I think this isn't quite right. Sure, they are free to host. But now we will simply always drop them?
I think we should still hoist all identical debug info, and skip the rest of the debuginfo.

[fhahn]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1442

I think we should now handle dbg intrinsics the same as if hoisting was enabled, because we just delegate the hoisting to the existing code. Here we just skip debug intrinsics for the check whether the only non-debug instruction is a branch. The loop should hoist any debug intrinsics if it can, because we do not change I1 and I2.

In hoist_with_debug2 the dbg.value calls are hoisted. They are dropped for hoist_with_debug3_pr49982, but I think not by the hoisting cod but another transform in SimplifyCFG, as they are also dropped without this patch.

[lebedev.ri]

LGTM, thanks.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

bool SimplifyCFGOpt::HoistThenElseCodeToIf(BranchInst *BI,
   auto _ = make_scope_exit([&]() {
     if (Changed)
       ++NumHoistCommonCode;
   });
 
   // Check if only hoisting terminators is allowed. This does not add new
   // instructions to the hoist location.
   if (EqTermsOnly) {
-    if (!I1->isIdenticalToWhenDefined(I2))
+    // Skip any debug intrinsics, as they are free to hoist.
+    auto *I1NonDbg = &*skipDebugIntrinsics(I1->getIterator());
+    auto *I2NonDbg = &*skipDebugIntrinsics(I2->getIterator());
+    if (!I1NonDbg->isIdenticalToWhenDefined(I2NonDbg))
       return false;
-    if (!I1->isTerminator())
+    if (!I1NonDbg->isTerminator())
       return false;
-    goto HoistTerminator;
+    // Now we know that we only need to hoist debug instrinsics and the
+    // terminator. Let the loop below handle those 2 cases.
   }
 
   do {
     // If we are hoisting the terminator instruction, don't move one (making a
     // broken BB), instead clone it, and remove BI.
     if (I1->isTerminator())
       goto HoistTerminator;
 

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll

exit:
   %k.0 = phi i32 [ %call.1, %then ], [ %call.2, %else ]
   ret i32 %k.0, !dbg !19
 }
 
 define i1 @hoist_with_debug2(i32 %x) !dbg !22 {
 ; CHECK-LABEL: @hoist_with_debug2(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp ugt i32 [[X:%.*]], 2
-; CHECK-NEXT:    [[P:%.*]] = select i1 [[TOBOOL_NOT]], i1 false, i1 true
-; CHECK-NEXT:    ret i1 [[P]]
+; CHECK-NEXT:    call void @llvm.dbg.value(metadata i32 [[X]], metadata [[META21:![0-9]+]], metadata !DIExpression()), !dbg [[DBG23:![0-9]+]]
+; CHECK-NEXT:    call void @llvm.dbg.value(metadata i32 [[X]], metadata [[META21]], metadata !DIExpression()), !dbg [[DBG23]]
+; CHECK-NEXT:    [[DOT:%.*]] = select i1 [[TOBOOL_NOT]], i1 false, i1 true
+; CHECK-NEXT:    ret i1 [[DOT]]
 ;
 entry:
   %tobool.not = icmp ugt i32 %x, 2
   br i1 %tobool.not, label %cond.end.thread, label %cond.end.thread19
 
 cond.end.thread19:                                ; preds = %land.lhs.true
   call void @llvm.dbg.value(metadata i32 %x, metadata !24, metadata !DIExpression()), !dbg !25
   br label %exit

 
 ; Test case for PR49982.
 define i16 @hoist_with_debug3_pr49982(i32 %x, i1 %c.2) !dbg !26 {
 ; CHECK-LABEL: @hoist_with_debug3_pr49982(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[FOR_COND:%.*]]
 ; CHECK:       for.cond:
 ; CHECK-NEXT:    [[C_0:%.*]] = icmp sgt i32 [[X:%.*]], 0
-; CHECK-NEXT:    br i1 [[C_0]], label [[CHECK:%.*]], label [[LATCH:%.*]]
-; CHECK:       check:
-; CHECK-NEXT:    [[C_1:%.*]] = icmp ugt i32 [[X]], 2
-; CHECK-NEXT:    br label [[EXIT_1:%.*]]
-; CHECK:       latch:
-; CHECK-NEXT:    br i1 [[C_2:%.*]], label [[EXIT_1]], label [[FOR_COND]]
+; CHECK-NEXT:    [[BRMERGE:%.*]] = or i1 [[C_0]], [[C_2:%.*]]
+; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[C_0]], i16 0, i16 20
+; CHECK-NEXT:    br i1 [[BRMERGE]], label [[EXIT_1:%.*]], label [[FOR_COND]]
 ; CHECK:       exit.1:
-; CHECK-NEXT:    [[MERGE:%.*]] = phi i16 [ 20, [[LATCH]] ], [ 0, [[CHECK]] ]
-; CHECK-NEXT:    ret i16 [[MERGE]]
+; CHECK-NEXT:    ret i16 [[DOTMUX]]
 ;
 entry:
   br label %for.cond
 
 for.cond:
   %c.0 = icmp sgt i32 %x, 0
   br i1 %c.0, label %check, label %latch