[PartialInliner] Fix crash when inlining functions with unreachable blocks

[davide]

Summary

CodeExtractor looks up the dominator node corresponding to return blocks when splitting them. If one of these blocks is unreachable, there's no node in the Dom and CodeExtractor crashes because it doesn't check for domtree node validity.
In theory, we could add just a check for skipping DTNode == nullptr in splitReturnBlock but the fix I propose here is slightly different. To the best of my knowledge, unreachable blocks are irrelevant for the algorithm, therefore we can just skip them when building the candidate set in the constructor.
I'm not 100% sure if any pass using CodeExtractor could make use of extracting an unreachable block (including bugpoint), in which case my fix would need to be adjusted.

[davidxl]

The root cause of the problem is that the partial inliner split the newreturn block unconditionally -- even when there is no PHIs in them (creating unreachable block). I have included that fix in my patch, but this patch is fine too.

lgtm

[chandlerc]

LGTM as well -- no sense doing anything with the unreachable blocks.

lib/Transforms/Utils/CodeExtractor.cpp

if (const CallInst *CI = dyn_cast<CallInst>(I))
           return false;
   }
 
   return true;
 }
 
 /// \brief Build a set of blocks to extract if the input blocks are viable.
 static SetVector<BasicBlock *>
-buildExtractionBlockSet(ArrayRef<BasicBlock *> BBs) {
-  auto BBBegin = BBs.begin();
-  auto BBEnd = BBs.end();
-  assert(BBBegin != BBEnd && "The set of blocks to extract must be non-empty");
-
+buildExtractionBlockSet(ArrayRef<BasicBlock *> BBs, DominatorTree *DT) {
+  assert(!BBs.empty() && "The set of blocks to extract must be non-empty");
   SetVector<BasicBlock *> Result;
 
   // Loop over the blocks, adding them to our set-vector, and aborting with an
   // empty set if we encounter invalid blocks.
-  do {
-    if (!Result.insert(*BBBegin))
-      llvm_unreachable("Repeated basic blocks in extraction input");
+  for (BasicBlock *BB : BBs) {
+
+    // If this block is dead, don't process it.
+    if (DT && !DT->isReachableFromEntry(BB))
+      continue;
 
-    if (!CodeExtractor::isBlockValidForExtraction(**BBBegin)) {
+    if (!Result.insert(BB))
+      llvm_unreachable("Repeated basic blocks in extraction input");
+    if (!CodeExtractor::isBlockValidForExtraction(*BB)) {
       Result.clear();
       return Result;
     }
-  } while (++BBBegin != BBEnd);
+  }
 
 #ifndef NDEBUG
   for (SetVector<BasicBlock *>::iterator I = std::next(Result.begin()),
                                          E = Result.end();
        I != E; ++I)
     for (pred_iterator PI = pred_begin(*I), PE = pred_end(*I);
          PI != PE; ++PI)
       assert(Result.count(*PI) &&
              "No blocks in this region may have entries from outside the region"
              " except for the first block!");
 #endif
 
   return Result;
 }
 
 CodeExtractor::CodeExtractor(ArrayRef<BasicBlock *> BBs, DominatorTree *DT,
                              bool AggregateArgs, BlockFrequencyInfo *BFI,
                              BranchProbabilityInfo *BPI)
     : DT(DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
-      BPI(BPI), Blocks(buildExtractionBlockSet(BBs)), NumExitBlocks(~0U) {}
+      BPI(BPI), Blocks(buildExtractionBlockSet(BBs, DT)), NumExitBlocks(~0U) {}
 
 CodeExtractor::CodeExtractor(DominatorTree &DT, Loop &L, bool AggregateArgs,
                              BlockFrequencyInfo *BFI,
                              BranchProbabilityInfo *BPI)
     : DT(&DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
-      BPI(BPI), Blocks(buildExtractionBlockSet(L.getBlocks())),
+      BPI(BPI), Blocks(buildExtractionBlockSet(L.getBlocks(), &DT)),
       NumExitBlocks(~0U) {}
 
 /// definedInRegion - Return true if the specified value is defined in the
 /// extracted region.
 static bool definedInRegion(const SetVector<BasicBlock *> &Blocks, Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V))
     if (Blocks.count(I->getParent()))
       return true;

test/Transforms/CodeExtractor/unreachable-block.ll

+; RUN: opt -S -partial-inliner %s | FileCheck %s
+
+; CHECK-LABEL: define void @dipsy(
+; CHECK-NEXT:   call void @tinkywinky.1_ontrue()
+; CHECK-NEXT:   call void @patatuccio()
+; CHECK-NEXT:   ret void
+; CHECK-NEXT: }
+
+; CHECK-LABEL: define internal void @tinkywinky.1_ontrue() {
+; CHECK-NEXT: newFuncRoot:
+; CHECK-NEXT:   br label %ontrue
+; CHECK: .exitStub:
+; CHECK-NEXT:   ret void
+; CHECK: ontrue:
+; CHECK-NEXT:   call void @patatino()
+; CHECK-NEXT:   br label %onfalse
+; CHECK: onfalse:
+; CHECK-NEXT:   br label %.exitStub
+; CHECK-NEXT: }
+
+declare void @patatino()
+declare void @patatuccio()
+
+define fastcc void @tinkywinky() {
+  br i1 true, label %ontrue, label %onfalse
+ontrue:
+  call void @patatino()
+  br label %onfalse
+onfalse:
+  call void @patatuccio()
+  ret void
+cantreachme:
+  ret void
+}
+define void @dipsy() {
+  call fastcc void @tinkywinky()
+  ret void
+}