[globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC

[dsanders]

Summary

There's little of interest that can be done to an already-erased instruction.
You can't inspect it, write it to a debug log, etc. It ought to be notification
that we're about to erase it. Rename the function to clarify the timing of the
event and reflect current usage.

Also fixed one case where we were trying to print an erased instruction.

[aditya_nandakumar]

Thanks. LGTM.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h

 class MachineRegisterInfo;
 class MachineInstr;
 
 class CombinerHelper {
   MachineIRBuilder &Builder;
   MachineRegisterInfo &MRI;
   GISelChangeObserver &Observer;
 
-  void eraseInstr(MachineInstr &MI);
   void scheduleForVisit(MachineInstr &MI);
 
 public:
   CombinerHelper(GISelChangeObserver &Observer, MachineIRBuilder &B);
 
   /// If \p MI is COPY, try to combine it.
   /// Returns true if MI changed.
   bool tryCombineCopy(MachineInstr &MI);

include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h

 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CODEGEN_GLOBALISEL_GISELCHANGEOBSERVER_H
 #define LLVM_CODEGEN_GLOBALISEL_GISELCHANGEOBSERVER_H
 
 namespace llvm {
 /// Abstract class that contains various methods for clients to notify about
 /// changes. This should be the preferred way for APIs to notify changes.
-/// Typically calling erasedInstr/createdInstr multiple times should not affect
+/// Typically calling erasingInstr/createdInstr multiple times should not affect
 /// the result. The observer would likely need to check if it was already
 /// notified earlier (consider using GISelWorkList).
 class MachineInstr;
 class GISelChangeObserver {
 public:
   virtual ~GISelChangeObserver() {}
 
-  /// An instruction was erased.
-  virtual void erasedInstr(MachineInstr &MI) = 0;
+  /// An instruction is about to be erased.
+  virtual void erasingInstr(MachineInstr &MI) = 0;
   /// An instruction was created and inserted into the function.
   virtual void createdInstr(MachineInstr &MI) = 0;
   /// This instruction was mutated in some way.
   virtual void changedInstr(MachineInstr &MI) = 0;
 };
 
 } // namespace llvm
 #endif

lib/CodeGen/GlobalISel/Combiner.cpp

 class WorkListMaintainer : public GISelChangeObserver {
   using WorkListTy = GISelWorkList<512>;
   WorkListTy &WorkList;
 
 public:
   WorkListMaintainer(WorkListTy &WorkList) : WorkList(WorkList) {}
   virtual ~WorkListMaintainer() {}
 
-  void erasedInstr(MachineInstr &MI) override {
+  void erasingInstr(MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Erased: "; MI.print(dbgs()); dbgs() << "\n");
     WorkList.remove(&MI);
   }
   void createdInstr(MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Created: "; MI.print(dbgs()); dbgs() << "\n");
     WorkList.insert(&MI);
   }
   // Currently changed conservatively assumes erased.

lib/CodeGen/GlobalISel/CombinerHelper.cpp

 #define DEBUG_TYPE "gi-combine"
 
 using namespace llvm;
 
 CombinerHelper::CombinerHelper(GISelChangeObserver &Observer,
                                MachineIRBuilder &B)
     : Builder(B), MRI(Builder.getMF().getRegInfo()), Observer(Observer) {}
 
-void CombinerHelper::eraseInstr(MachineInstr &MI) {
-  Observer.erasedInstr(MI);
-}
 void CombinerHelper::scheduleForVisit(MachineInstr &MI) {
   Observer.createdInstr(MI);
 }
 
 bool CombinerHelper::tryCombineCopy(MachineInstr &MI) {
   if (MI.getOpcode() != TargetOpcode::COPY)
     return false;
   unsigned DstReg = MI.getOperand(0).getReg();
for (auto &InsertionInfo : ScheduleForInsert) {
     Builder.setInsertPt(*InsertIntoBB, InsertBefore);
     unsigned NewDstReg = MRI.cloneVirtualRegister(MI.getOperand(0).getReg());
     MachineInstr *NewMI = Builder.buildTrunc(NewDstReg, ChosenDstReg);
     EmittedInsns[InsertIntoBB] = NewMI;
     UseMO->setReg(NewDstReg);
     Observer.createdInstr(*NewMI);
   }
   for (auto &EraseMI : ScheduleForErase) {
+    Observer.erasingInstr(*EraseMI);
     EraseMI->eraseFromParent();
-    Observer.erasedInstr(*EraseMI);
   }
   MI.getOperand(0).setReg(ChosenDstReg);
 
   return true;
 }
 
 bool CombinerHelper::tryCombine(MachineInstr &MI) {
   if (tryCombineCopy(MI))
     return true;
   return tryCombineExtendingLoads(MI);
 }

lib/CodeGen/GlobalISel/Legalizer.cpp

if (isPreISelGenericOpcode(MI.getOpcode())) {
       if (isArtifact(MI))
         ArtifactList.insert(&MI);
       else
         InstList.insert(&MI);
     }
     LLVM_DEBUG(dbgs() << ".. .. New MI: " << MI;);
   }
 
-  void erasedInstr(MachineInstr &MI) override {
+  void erasingInstr(MachineInstr &MI) override {
+    LLVM_DEBUG(dbgs() << ".. .. Erasing: " << MI;);
     InstList.remove(&MI);
     ArtifactList.remove(&MI);
   }
 
   void changedInstr(MachineInstr &MI) override {
     // When insts change, we want to revisit them to legalize them again.
     // We'll consider them the same as created.
     LLVM_DEBUG(dbgs() << ".. .. Changed MI: " << MI;);
for (MachineInstr &MI : *MBB) {
         InstList.insert(&MI);
     }
   }
   LegalizerWorkListManager WorkListObserver(InstList, ArtifactList);
   LegalizerHelper Helper(MF, WorkListObserver);
   const LegalizerInfo &LInfo(Helper.getLegalizerInfo());
   LegalizationArtifactCombiner ArtCombiner(Helper.MIRBuilder, MF.getRegInfo(), LInfo);
   auto RemoveDeadInstFromLists = [&WorkListObserver](MachineInstr *DeadMI) {
-    WorkListObserver.erasedInstr(*DeadMI);
+    WorkListObserver.erasingInstr(*DeadMI);
   };
   bool Changed = false;
   do {
     while (!InstList.empty()) {
       MachineInstr &MI = *InstList.pop_back_val();
       assert(isPreISelGenericOpcode(MI.getOpcode()) && "Expecting generic opcode");
       if (isTriviallyDead(MI, MRI)) {
         LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
while (!InstList.empty()) {
         return false;
       }
       Changed |= Res == LegalizerHelper::Legalized;
     }
     while (!ArtifactList.empty()) {
       MachineInstr &MI = *ArtifactList.pop_back_val();
       assert(isPreISelGenericOpcode(MI.getOpcode()) && "Expecting generic opcode");
       if (isTriviallyDead(MI, MRI)) {
-        LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
+        LLVM_DEBUG(dbgs() << MI << "Is dead\n");
         RemoveDeadInstFromLists(&MI);
         MI.eraseFromParentAndMarkDBGValuesForRemoval();
         continue;
       }
       SmallVector<MachineInstr *, 4> DeadInstructions;
       if (ArtCombiner.tryCombineInstruction(MI, DeadInstructions)) {
         for (auto *DeadMI : DeadInstructions) {
-          LLVM_DEBUG(dbgs() << ".. Erasing Dead Instruction " << *DeadMI);
+          LLVM_DEBUG(dbgs() << *DeadMI << "Is dead\n");
           RemoveDeadInstFromLists(DeadMI);
           DeadMI->eraseFromParentAndMarkDBGValuesForRemoval();
         }
         Changed = true;
         continue;
       }
       // If this was not an artifact (that could be combined away), this might
       // need special handling. Add it to InstList, so when it's processed

unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

 #include "LegalizerHelperTest.h"
 
 namespace {
 
 class DummyGISelObserver : public GISelChangeObserver {
 public:
   void changedInstr(MachineInstr &MI) override {}
   void createdInstr(MachineInstr &MI) override {}
-  void erasedInstr(MachineInstr &MI) override {}
+  void erasingInstr(MachineInstr &MI) override {}
 };
 
 // Test CTTZ expansion when CTTZ_ZERO_UNDEF is legal or custom,
 // in which case it becomes CTTZ_ZERO_UNDEF with select.
 TEST_F(LegalizerHelperTest, LowerBitCountingCTTZ0) {
   if (!TM)
     return;