[IR] Don't allow successors() over block without terminators

llvm/include/llvm/IR/CFG.h

@@ -141,28 +141,24 @@ using succ_range = iterator_range<succ_iterator>;
 using const_succ_range = iterator_range<const_succ_iterator>;
 
 inline succ_iterator succ_begin(Instruction *I) {
-  return I ? I->successors().begin() : succ_iterator(nullptr);
+  return I->successors().begin();
 }
 inline const_succ_iterator succ_begin(const Instruction *I) {
-  return I ? I->successors().begin() : const_succ_iterator(nullptr);
-}
-inline succ_iterator succ_end(Instruction *I) {
-  return I ? I->successors().end() : succ_iterator(nullptr);
+  return I->successors().begin();
 }
+inline succ_iterator succ_end(Instruction *I) { return I->successors().end(); }
 inline const_succ_iterator succ_end(const Instruction *I) {
-  return I ? I->successors().end() : const_succ_iterator(nullptr);
+  return I->successors().end();
 }
 inline bool succ_empty(const Instruction *I) {
   return succ_begin(I) == succ_end(I);
 }
 inline unsigned succ_size(const Instruction *I) {
   return std::distance(succ_begin(I), succ_end(I));
 }
-inline succ_range successors(Instruction *I) {
-  return succ_range(succ_begin(I), succ_end(I));
-}
+inline succ_range successors(Instruction *I) { return I->successors(); }
 inline const_succ_range successors(const Instruction *I) {
-  return const_succ_range(succ_begin(I), succ_end(I));
+  return I->successors();
 }
 
 inline succ_iterator succ_begin(BasicBlock *BB) {
@@ -184,10 +180,10 @@ inline unsigned succ_size(const BasicBlock *BB) {
   return std::distance(succ_begin(BB), succ_end(BB));
 }
 inline succ_range successors(BasicBlock *BB) {
-  return succ_range(succ_begin(BB), succ_end(BB));
+  return successors(BB->getTerminator());
 }
 inline const_succ_range successors(const BasicBlock *BB) {
-  return const_succ_range(succ_begin(BB), succ_end(BB));
+  return successors(BB->getTerminator());
 }
 
 //===--------------------------------------------------------------------===//

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

@@ -5585,11 +5585,18 @@ OpenMPIRBuilder::applyStaticChunkedWorkshareLoop(
   Constant *One = ConstantInt::get(InternalIVTy, 1);
 
   Function *F = CLI->getFunction();
+  // Blocks must have terminators.
+  SmallVector<Instruction *> UIs;
+  for (BasicBlock &BB : *F)
+    if (!BB.getTerminator())
+      UIs.push_back(new UnreachableInst(F->getContext(), &BB));
   FunctionAnalysisManager FAM;
   FAM.registerPass([]() { return DominatorTreeAnalysis(); });
   FAM.registerPass([]() { return PassInstrumentationAnalysis(); });
   LoopAnalysis LIA;
   LoopInfo &&LI = LIA.run(*F, FAM);
+  for (Instruction *I : UIs)
+    I->eraseFromParent();
   Loop *L = LI.getLoopFor(CLI->getHeader());
   SmallVector<Metadata *> LoopMDList;
   if (ChunkSize || DistScheduleChunkSize)
@@ -6874,6 +6881,12 @@ void OpenMPIRBuilder::applySimd(CanonicalLoopInfo *CanonicalLoop,
 
   Function *F = CanonicalLoop->getFunction();
 
+  // Blocks must have terminators.
+  SmallVector<Instruction *> UIs;
+  for (BasicBlock &BB : *F)
+    if (!BB.getTerminator())
+      UIs.push_back(new UnreachableInst(F->getContext(), &BB));
+
   // TODO: We should not rely on pass manager. Currently we use pass manager
   // only for getting llvm::Loop which corresponds to given CanonicalLoopInfo
   // object. We should have a method  which returns all blocks between
@@ -6886,6 +6899,9 @@ void OpenMPIRBuilder::applySimd(CanonicalLoopInfo *CanonicalLoop,
   LoopAnalysis LIA;
   LoopInfo &&LI = LIA.run(*F, FAM);
 
+  for (Instruction *I : UIs)
+    I->eraseFromParent();
+
   Loop *L = LI.getLoopFor(CanonicalLoop->getHeader());
   if (AlignedVars.size()) {
     InsertPointTy IP = Builder.saveIP();
@@ -7003,6 +7019,12 @@ static int32_t computeHeuristicUnrollFactor(CanonicalLoopInfo *CLI) {
   CodeGenOptLevel OptLevel = CodeGenOptLevel::Aggressive;
   std::unique_ptr<TargetMachine> TM = createTargetMachine(F, OptLevel);
 
+  // Blocks must have terminators.
+  SmallVector<Instruction *> UIs;
+  for (BasicBlock &BB : *F)
+    if (!BB.getTerminator())
+      UIs.push_back(new UnreachableInst(F->getContext(), &BB));
+
   FunctionAnalysisManager FAM;
   FAM.registerPass([]() { return TargetLibraryAnalysis(); });
   FAM.registerPass([]() { return AssumptionAnalysis(); });
@@ -7027,6 +7049,9 @@ static int32_t computeHeuristicUnrollFactor(CanonicalLoopInfo *CLI) {
   AssumptionCache &&AC = ACT.run(*F, FAM);
   OptimizationRemarkEmitter ORE{F};
 
+  for (Instruction *I : UIs)
+    I->eraseFromParent();
+
   Loop *L = LI.getLoopFor(CLI->getHeader());
   assert(L && "Expecting CanonicalLoopInfo to be recognized as a loop");
 

llvm/lib/FuzzMutate/RandomIRBuilder.cpp

@@ -21,11 +21,23 @@
 using namespace llvm;
 using namespace fuzzerop;
 
+static DominatorTree getDomTree(Function &F) {
+  // Dominator tree construction requires that all blocks have terminators.
+  SmallVector<Instruction *> AddedInsts;
+  for (BasicBlock &BB : F)
+    if (!BB.getTerminator())
+      AddedInsts.push_back(new UnreachableInst(F.getContext(), &BB));
+  DominatorTree DT(F);
+  for (Instruction *I : AddedInsts)
+    I->eraseFromParent();
+  return DT;
+}
+
 /// Return a vector of Blocks that dominates this block, excluding current
 /// block.
 static std::vector<BasicBlock *> getDominators(BasicBlock *BB) {
   std::vector<BasicBlock *> ret;
-  DominatorTree DT(*BB->getParent());
+  DominatorTree DT = getDomTree(*BB->getParent());
   DomTreeNode *Node = DT.getNode(BB);
   // It's possible that an orphan block is not in the dom tree. In that case we
   // just return nothing.
@@ -43,7 +55,7 @@ static std::vector<BasicBlock *> getDominators(BasicBlock *BB) {
 /// Return a vector of Blocks that is dominated by this block, excluding current
 /// block
 static std::vector<BasicBlock *> getDominatees(BasicBlock *BB) {
-  DominatorTree DT(*BB->getParent());
+  DominatorTree DT = getDomTree(*BB->getParent());
   std::vector<BasicBlock *> ret;
   DomTreeNode *Parent = DT.getNode(BB);
   // It's possible that an orphan block is not in the dom tree. In that case we

llvm/lib/IR/Verifier.cpp

@@ -410,10 +410,9 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
     // pass manager to provide this as it isolates us from a potentially
     // out-of-date dominator tree and makes it significantly more complex to run
     // this code outside of a pass manager.
-    // FIXME: It's really gross that we have to cast away constness here.
-    if (!F.empty())
-      DT.recalculate(const_cast<Function &>(F));
 
+    // First check that every basic block has a terminator, otherwise we can't
+    // even inspect the CFG.
     for (const BasicBlock &BB : F) {
       if (!BB.empty() && BB.back().isTerminator())
         continue;
@@ -427,6 +426,10 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
       return false;
     }
 
+    // FIXME: It's really gross that we have to cast away constness here.
+    if (!F.empty())
+      DT.recalculate(const_cast<Function &>(F));
+
     auto FailureCB = [this](const Twine &Message) {
       this->CheckFailed(Message);
     };

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

@@ -346,6 +346,8 @@ bool llvm::MergeBlockIntoPredecessor(BasicBlock *BB, DomTreeUpdater *DTU,
   if (PredecessorWithTwoSuccessors) {
     // Delete the unconditional branch from BB.
     BB->back().eraseFromParent();
+    // Add unreachable to now empty BB.
+    new UnreachableInst(BB->getContext(), BB);
 
     // Update branch in the predecessor.
     PredBB_BI->setSuccessor(FallThruPath, NewSucc);
@@ -355,15 +357,15 @@ bool llvm::MergeBlockIntoPredecessor(BasicBlock *BB, DomTreeUpdater *DTU,
 
     // Move terminator instruction.
     BB->back().moveBeforePreserving(*PredBB, PredBB->end());
+    // Add unreachable to now empty BB.
+    new UnreachableInst(BB->getContext(), BB);
 
     // Terminator may be a memory accessing instruction too.
     if (MSSAU)
       if (MemoryUseOrDef *MUD = cast_or_null<MemoryUseOrDef>(
               MSSAU->getMemorySSA()->getMemoryAccess(PredBB->getTerminator())))
         MSSAU->moveToPlace(MUD, PredBB, MemorySSA::End);
   }
-  // Add unreachable to now empty BB.
-  new UnreachableInst(BB->getContext(), BB);
 
   // Inherit predecessors name if it exists.
   if (!PredBB->hasName())

llvm/test/Transforms/LoopSimplifyCFG/mssa_term.ll

@@ -0,0 +1,46 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -passes="loop-mssa(loop-simplifycfg,simple-loop-unswitch)" < %s | FileCheck %s
+
+; Check that IR is valid when MemorySSA is updated during MergeBlockIntoPredecessor.
+
+define i32 @f1(i1 %cond) personality ptr null {
+; CHECK-LABEL: define i32 @f1(
+; CHECK-SAME: i1 [[COND:%.*]]) personality ptr null {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    br i1 [[COND]], label %[[ENTRY_SPLIT:.*]], label %[[COMMON_RET_LOOPEXIT:.*]]
+; CHECK:       [[ENTRY_SPLIT]]:
+; CHECK-NEXT:    br label %[[FOR_COND:.*]]
+; CHECK:       [[FOR_COND]]:
+; CHECK-NEXT:    [[CALL26:%.*]] = invoke i32 @f2(ptr null, ptr null, ptr null)
+; CHECK-NEXT:            to label %[[FOR_COND]] unwind label %[[LPAD24:.*]]
+; CHECK:       [[COMMON_RET_LOOPEXIT]]:
+; CHECK-NEXT:    br label %[[COMMON_RET:.*]]
+; CHECK:       [[COMMON_RET]]:
+; CHECK-NEXT:    [[COMMON_RET_OP:%.*]] = phi i32 [ 0, %[[LPAD24]] ], [ 0, %[[COMMON_RET_LOOPEXIT]] ]
+; CHECK-NEXT:    ret i32 [[COMMON_RET_OP]]
+; CHECK:       [[LPAD24]]:
+; CHECK-NEXT:    [[LPAD:%.*]] = landingpad { ptr, i32 }
+; CHECK-NEXT:            cleanup
+; CHECK-NEXT:    br label %[[COMMON_RET]]
+;
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %if.end19, %entry
+  br i1 %cond, label %if.end19, label %common.ret
+
+common.ret:                                       ; preds = %lpad24, %for.cond
+  %common.ret.op = phi i32 [ 0, %lpad24 ], [ 0, %for.cond ]
+  ret i32 %common.ret.op
+
+if.end19:                                         ; preds = %for.cond
+  %call26 = invoke i32 @f2(ptr null, ptr null, ptr null)
+  to label %for.cond unwind label %lpad24
+
+lpad24:                                           ; preds = %if.end19
+  %lpad = landingpad { ptr, i32 }
+  cleanup
+  br label %common.ret
+}
+
+declare i32 @f2(ptr, ptr, ptr)

llvm/unittests/Analysis/BasicAliasAnalysisTest.cpp

@@ -79,6 +79,7 @@ TEST_F(BasicAATest, AliasInstWithObjectOfImpreciseSize) {
 
   BasicBlock *Entry(BasicBlock::Create(C, "", F));
   B.SetInsertPoint(Entry);
+  B.CreateRetVoid();
 
   Value *IncomingI32Ptr = F->arg_begin();
 
@@ -119,6 +120,7 @@ TEST_F(BasicAATest, AliasInstWithFullObjectOfImpreciseSize) {
   AllocaInst *I8 = B.CreateAlloca(B.getInt8Ty(), B.getInt32(2));
   auto *I8AtUncertainOffset =
       cast<GetElementPtrInst>(B.CreatePtrAdd(I8, ArbitraryI32));
+  B.CreateRetVoid();
 
   auto &AllAnalyses = setupAnalyses();
   BasicAAResult &BasicAA = AllAnalyses.BAA;

llvm/unittests/Analysis/DomTreeUpdaterTest.cpp

@@ -776,6 +776,7 @@ TEST(DomTreeUpdater, LazyUpdateDeduplicationTest) {
   // CFG Change: remove bb0 -> bb1.
   EXPECT_EQ(BB0->getTerminator()->getNumSuccessors(), 1u);
   BB0->getTerminator()->eraseFromParent();
+  new UnreachableInst(Context, BB0);
 
   // Update the DTU and simulate invalid updates.
   DTU.applyUpdatesPermissive({{DominatorTree::Delete, BB0, BB1},