[SimpleLoopUnswitch] Update DefaultExit condition to check unreachable is not empty.

[asbirlea]

Summary

Update the check for the default exit block to not only check that the
terminator is not unreachable, but also check that unreachable block has
*only* the unreachable instruction.

[asbirlea]

Gentle ping.

[chandlerc]

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

601

Rather than trying to name after the implementation, how about naming after the usage? IsTriviallyUnswitchableExitBlock?

I'd also use a reference here, but that's a pretty minor point.

602–609

I'd rewrite this to use early exit rather than combining flags, and add some comments about the non-obvious part (IMO, the non-empty unreachable).

Something like:

if (L.contains(BBToCheck))
  return false;
if (!areLoopExitPHIsLoopInvariant(L, *ParentBB, *BBToCheck))
  return false;
....

[asbirlea]

Address comments.

[efriedma]

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

614

Need to skip debug info

[chandlerc]

LGTM with the addition of skipping debug info as Eli suggests.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

614

I hadn't expected that to matter as the thing we care most about here are the empty-but-unreachable blocks *created* by this same pass (and those would never have debug info).

But I suppose you're right that we still need to skip debug info because then the frontend adding it could suddenly enable (pointless) unswitching.

Ugh, but good catch.

[asbirlea]

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

614

Just to clarify, when TI is an unreachable, the check should instead traverse BBToCheck and verify that if any instructions beside TI exists, they are an IntrinsicInstr and I->getIntrinsicID() is one of: Intrinsic::dbg_addr, Intrinsic::dbg_declare, Intrinsic::dbg_label or Intrinsic::dbg_value.
Is this right, or is there a simpler way to add this check?

Thanks for the input!

[efriedma]

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

614

BBToCheck()->getFirstNonPHIOrDbg() != TI?

[asbirlea]

Address comment.
Thank you for the clarifications!

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
   Value *LoopCond = SI.getCondition();
 
   // If this isn't switching on an invariant condition, we can't unswitch it.
   if (!L.isLoopInvariant(LoopCond))
     return false;
 
   auto *ParentBB = SI.getParent();
 
+  auto IsTriviallyUnswitchableExitBlock = [&](BasicBlock &BBToCheck) {
+    // BBToCheck is not an exit block if it is inside loop L.
+    if (L.contains(&BBToCheck))
+      return false;
+    // BBToCheck is not trivial to unswitch if its phis aren't loop invariant.
+    if (!areLoopExitPHIsLoopInvariant(L, *ParentBB, BBToCheck))
+      return false;
+    // We do not unswitch a block that only has an unreachable statement, as
+    // it's possible this is a previously unswitched block. Only unswitch if
+    // either the terminator is not unreachable, or, if it is, it's not the only
+    // instruction in the block.
+    auto *TI = BBToCheck.getTerminator();
+    bool isUnreachable = isa<UnreachableInst>(TI);
+    return !isUnreachable ||
+           (isUnreachable && (BBToCheck.getFirstNonPHIOrDbg() != TI));
+  };
+
   SmallVector<int, 4> ExitCaseIndices;
   for (auto Case : SI.cases()) {
     auto *SuccBB = Case.getCaseSuccessor();
     if (!L.contains(SuccBB) &&
         areLoopExitPHIsLoopInvariant(L, *ParentBB, *SuccBB))
       ExitCaseIndices.push_back(Case.getCaseIndex());
   }
   BasicBlock *DefaultExitBB = nullptr;
   SwitchInstProfUpdateWrapper::CaseWeightOpt DefaultCaseWeight =
       SwitchInstProfUpdateWrapper::getSuccessorWeight(SI, 0);
-  if (!L.contains(SI.getDefaultDest()) &&
-      areLoopExitPHIsLoopInvariant(L, *ParentBB, *SI.getDefaultDest()) &&
-      !isa<UnreachableInst>(SI.getDefaultDest()->getTerminator())) {
+  if (IsTriviallyUnswitchableExitBlock(*SI.getDefaultDest())) {
     DefaultExitBB = SI.getDefaultDest();
   } else if (ExitCaseIndices.empty())
     return false;
 
   LLVM_DEBUG(dbgs() << "    unswitching trivial switch...\n");
 
   if (MSSAU && VerifyMemorySSA)
     MSSAU->getMemorySSA()->verifyMemorySSA();

llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch.ll

 ; CHECK-NEXT:    call void @sink(i32 %[[PHI]])
 ; CHECK-NEXT:    br label %header
 
 loopexit:
   ret void
 ; CHECK:       loopexit:
 ; CHECK-NEXT:    ret
 }
+
+declare void @f()
+declare void @g()
+define void @test_unswitch_switch_with_nonempty_unreachable() {
+; CHECK-LABEL: @test_unswitch_switch_with_nonempty_unreachable()
+entry:
+  br label %loop
+
+loop:
+  %cleanup.dest.slot.0 = select i1 undef, i32 5, i32 undef
+  br label %for.cond
+
+for.cond:
+  switch i32 %cleanup.dest.slot.0, label %NonEmptyUnreachableBlock [
+    i32 0, label %for.cond
+    i32 1, label %NonEmptyUnreachableBlock
+    i32 2, label %loop.loopexit
+  ]
+
+loop.loopexit:
+  unreachable
+
+NonEmptyUnreachableBlock:
+  call void @f()
+  call void @g()
+  unreachable
+
+; CHECK:loop:
+; CHECK-NEXT:  %cleanup.dest.slot.0 = select i1 undef, i32 5, i32 undef
+; CHECK-NEXT:  switch i32 %cleanup.dest.slot.0, label %NonEmptyUnreachableBlock [
+; CHECK-NEXT:    i32 1, label %NonEmptyUnreachableBlock
+; CHECK-NEXT:    i32 2, label %loop.loopexit
+; CHECK-NEXT:    i32 0, label %loop.split
+; CHECK-NEXT:  ]
+
+; CHECK:loop.split:
+; CHECK-NEXT:  br label %for.cond
+
+; CHECK:for.cond:
+; CHECK-NEXT:  br label %for.cond
+
+; CHECK:loop.loopexit:
+; CHECK-NEXT:  unreachable
+
+; CHECK:NonEmptyUnreachableBlock:
+; CHECK-NEXT:  call void @f()
+; CHECK-NEXT:  call void @g()
+; CHECK-NEXT:  unreachable
+}