[IR][NFC] Add BasicBlock::erase(Instruction *I) for erasing a single Instruction.

[nikic]

How does this differ from I->eraseFromParent()?

[aeubanks]

is this to use with Instruction::eraseFromParent? if so I think that change should also be in this patch to show why this patch exists

[vporpo]

It is exactly the same. There is no super strong reason for having both functions.
The main reason is that the BB behaves like a list as it implements begin(), end(), size(), splice() etc. so not having an erase(I) function looks strange.
Up until now one could simply do BB->getInstList().erase(I), instead of I->eraseFromParent(), so this function is covering this case.

[vporpo]

I->eraseFromParent() now calls BasicBlock::erase(BB). Also BasicBlock::erase(BB) now calls InstList.erase() directly instead of calling BasicBlock::erase(FromIt, ToIt).

[barannikov88]

In D140237#4002292, @vporpo wrote:

The main reason is that the BB behaves like a list as it implements begin(), end(), size(), splice() etc. so not having an erase(I) function looks strange.
Up until now one could simply do BB->getInstList().erase(I), instead of I->eraseFromParent(), so this function is covering this case.

If the intention is to make it behave like a container, then the erase method should take an iterator.
But who would possibly use it if it requires a BB reference to call the method? It is kind of easier and safer to just I->eraseFromParent().
I don't mind adding this, just don't see how it can be useful.

[vporpo]

If the intention is to make it behave like a container, then the erase method should take an iterator.
But who would possibly use it if it requires a BB reference to call the method? It is kind of easier and safer to just I->eraseFromParent().
I don't mind adding this, just don't see how it can be useful.

I agree, I think it is not very useful. I will abandon the patch.

llvm/include/llvm/IR/BasicBlock.h

public:
   void splice(BasicBlock::iterator ToIt, BasicBlock *FromBB,
               BasicBlock::iterator FromBeginIt,
               BasicBlock::iterator FromEndIt);
 
   /// Erases a range of instructions from \p FromIt to (not including) \p ToIt.
   /// \Returns \p ToIt.
   BasicBlock::iterator erase(BasicBlock::iterator FromIt, BasicBlock::iterator ToIt);
 
+  /// Erases \p I from the instruction list. \Returns the iterator of the
+  /// instruction following \p I.
+  BasicBlock::iterator erase(Instruction *I) {
+    return InstList.erase(I->getIterator());
+  }
+
   /// Returns true if there are any uses of this basic block other than
   /// direct branches, switches, etc. to it.
   bool hasAddressTaken() const {
     return getBasicBlockBits().BlockAddressRefCount != 0;
   }
 
   /// Update all phi nodes in this basic block to refer to basic block \p New
   /// instead of basic block \p Old.

llvm/lib/IR/Instruction.cpp

const Function *Instruction::getFunction() const {
   return getParent()->getParent();
 }
 
 void Instruction::removeFromParent() {
   getParent()->getInstList().remove(getIterator());
 }
 
 iplist<Instruction>::iterator Instruction::eraseFromParent() {
-  return getParent()->getInstList().erase(getIterator());
+  return getParent()->erase(this);
 }
 
 /// Insert an unlinked instruction into a basic block immediately before the
 /// specified instruction.
 void Instruction::insertBefore(Instruction *InsertPos) {
   insertInto(InsertPos->getParent(), InsertPos->getIterator());
 }
 

llvm/unittests/IR/BasicBlockTest.cpp

)");
   BB0->erase(Instr1->getIterator(), Instr2->getIterator());
   EXPECT_EQ(BB0->size(), 2u);
   EXPECT_EQ(&*BB0->begin(), Instr2);
 
   // Erase all instructions
   BB0->erase(BB0->begin(), BB0->end());
   EXPECT_TRUE(BB0->empty());
 }
+
+TEST(BasicBlockTest, EraseSingleInstr) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> M = parseIR(Ctx, R"(
+    define void @f(i32 %a) {
+     bb0:
+       %instr1 = add i32 %a, %a
+       %instr2 = sub i32 %a, %a
+       ret void
+    }
+)");
+  Function *F = &*M->begin();
+
+  auto BB0It = F->begin();
+  BasicBlock *BB0 = &*BB0It;
+
+  auto It = BB0->begin();
+  Instruction *Instr1 = &*It++;
+  Instruction *Instr2 = &*It++;
+  Instruction *Ret = &*It++;
+
+  EXPECT_EQ(BB0->size(), 3u);
+
+  // Erase %instr1
+  BB0->erase(Instr1);
+  EXPECT_EQ(BB0->size(), 2u);
+  EXPECT_EQ(&*BB0->begin(), Instr2);
+
+  // Erase ret
+  BB0->erase(Ret);
+  EXPECT_EQ(BB0->size(), 1u);
+  EXPECT_EQ(&*BB0->begin(), Instr2);
+
+  // Erase %instr2
+  BB0->erase(Instr2);
+  EXPECT_TRUE(BB0->empty());
+}
+
 } // End anonymous namespace.
 } // End llvm namespace.