Cowardly refuse to insert instructions after a terminator.

[arnt]

Summary

Allowing instructions to be added after a terminator just invites trouble later. Better to assert early, so the bug is easy to spot.

[arnt]

FWIW, I picked reviewers by taking the last two people who changed the file. I hope reviewing this isn't too much of an inconvenience ;)

[arnt]

LebedevRI pointed out that the assert ought to have a message; thanks.

[vsk]

Thanks for working on this. Two thoughts:

• Would the old test fail with the new assert? I don’t yet see why it would.
• Might help to preemptively test this with a stage 2 clang self-host, in case any in-tree code “fixes up” extra terminators.

[arnt]

The old test did break, yes, that's why I changed it at all. The six branch instructions I moved down were all followed by phi nodes.

I'll start a clang rebuild one night. Haven't done it yet.

llvm/lib/IR/Instruction.cpp

 }
 
 Instruction::Instruction(Type *ty, unsigned it, Use *Ops, unsigned NumOps,
                          BasicBlock *InsertAtEnd)
   : User(ty, Value::InstructionVal + it, Ops, NumOps), Parent(nullptr) {
 
   // append this instruction into the basic block
   assert(InsertAtEnd && "Basic block to append to may not be NULL!");
+  assert(!InsertAtEnd->getTerminator() && "Cannot append an instruction after a terminator");
   InsertAtEnd->getInstList().push_back(this);
 }
 
 Instruction::~Instruction() {
   assert(!Parent && "Instruction still linked in the program!");
   if (hasMetadataHashEntry())
     clearMetadataHashEntries();
 }

llvm/unittests/Analysis/PhiValuesTest.cpp

TEST(PhiValuesTest, SimplePhi) {
   // Create a function with phis that do not have other phis as incoming values
   Function *F = Function::Create(FunctionType::get(VoidTy, false),
                                  Function::ExternalLinkage, "f", M);
 
   BasicBlock *Entry = BasicBlock::Create(C, "entry", F);
   BasicBlock *If = BasicBlock::Create(C, "if", F);
   BasicBlock *Else = BasicBlock::Create(C, "else", F);
   BasicBlock *Then = BasicBlock::Create(C, "then", F);
-  BranchInst::Create(If, Else, UndefValue::get(I1Ty), Entry);
-  BranchInst::Create(Then, If);
-  BranchInst::Create(Then, Else);
-
   Value *Val1 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val1", Entry);
   Value *Val2 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val2", Entry);
   Value *Val3 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val3", Entry);
   Value *Val4 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val4", Entry);
 
+  BranchInst::Create(If, Else, UndefValue::get(I1Ty), Entry);
+  BranchInst::Create(Then, If);
+  BranchInst::Create(Then, Else);
+
   PHINode *Phi1 = PHINode::Create(I32Ty, 2, "phi1", Then);
   Phi1->addIncoming(Val1, If);
   Phi1->addIncoming(Val2, Else);
   PHINode *Phi2 = PHINode::Create(I32Ty, 2, "phi2", Then);
   Phi2->addIncoming(Val1, If);
   Phi2->addIncoming(Val3, Else);
 
   PhiValues PV(*F);
TEST(PhiValuesTest, DependentPhi) {
 
   BasicBlock *Entry = BasicBlock::Create(C, "entry", F);
   BasicBlock *If1 = BasicBlock::Create(C, "if1", F);
   BasicBlock *Else1 = BasicBlock::Create(C, "else1", F);
   BasicBlock *Then = BasicBlock::Create(C, "then", F);
   BasicBlock *If2 = BasicBlock::Create(C, "if2", F);
   BasicBlock *Else2 = BasicBlock::Create(C, "else2", F);
   BasicBlock *End = BasicBlock::Create(C, "then", F);
-  BranchInst::Create(If1, Else1, UndefValue::get(I1Ty), Entry);
-  BranchInst::Create(Then, If1);
-  BranchInst::Create(Then, Else1);
-  BranchInst::Create(If2, Else2, UndefValue::get(I1Ty), Then);
-  BranchInst::Create(End, If2);
-  BranchInst::Create(End, Else2);
 
   Value *Val1 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val1", Entry);
   Value *Val2 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val2", Entry);
   Value *Val3 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val3", Entry);
   Value *Val4 = new LoadInst(I32Ty, UndefValue::get(I32PtrTy), "val4", Entry);
 
+  BranchInst::Create(If1, Else1, UndefValue::get(I1Ty), Entry);
+  BranchInst::Create(Then, If1);
+  BranchInst::Create(Then, Else1);
+
   PHINode *Phi1 = PHINode::Create(I32Ty, 2, "phi1", Then);
   Phi1->addIncoming(Val1, If1);
   Phi1->addIncoming(Val2, Else1);
+
   PHINode *Phi2 = PHINode::Create(I32Ty, 2, "phi2", Then);
   Phi2->addIncoming(Val2, If1);
   Phi2->addIncoming(Val3, Else1);
+
+  BranchInst::Create(If2, Else2, UndefValue::get(I1Ty), Then);
+  BranchInst::Create(End, If2);
+  BranchInst::Create(End, Else2);
+
   PHINode *Phi3 = PHINode::Create(I32Ty, 2, "phi3", End);
   Phi3->addIncoming(Phi1, If2);
   Phi3->addIncoming(Val3, Else2);
 
   PhiValues PV(*F);
   PhiValues::ValueSet Vals;
 
   // Check that simple usage works