[GlobalISel] Don't remove from unfinalized GISelWorkList

[tobias-stadler]

Remove a hack from GISelWorkList caused by the Combiner removing
instructions from an unfinalized GISelWorkList during the DCE phase.
This is in preparation for larger changes to the WorkListMaintainer.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

Author: Tobias Stadler (tobias-stadler)

Changes

Remove a hack from GISelWorkList caused by the Combiner removing
instructions from an unfinalized GISelWorkList during the DCE phase.
This is in preparation for larger changes to the WorkListMaintainer.

---

Full diff: https://github.com/llvm/llvm-project/pull/102158.diff

3 Files Affected:

• (modified) llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h (+3)
• (modified) llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h (+1-1)
• (modified) llvm/lib/CodeGen/GlobalISel/Combiner.cpp (+6-5)

diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
index cad2216db34fe..7ec5dac9a6eba 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
@@ -79,6 +79,9 @@ class GISelObserverWrapper : public MachineFunction::Delegate,
     if (It != Observers.end())
       Observers.erase(It);
   }
+  // Removes all observers
+  void clearObservers() { Observers.clear(); }
+
   // API for Observer.
   void erasingInstr(MachineInstr &MI) override {
     for (auto &O : Observers)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h b/llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h
index 3ec6a1da201e2..dba3a8a14480c 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h
@@ -82,7 +82,7 @@ class GISelWorkList {
   /// Remove I from the worklist if it exists.
   void remove(const MachineInstr *I) {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
-    assert((Finalized || WorklistMap.empty()) && "Neither finalized nor empty");
+    assert(Finalized && "GISelWorkList used without finalizing");
 #endif
     auto It = WorklistMap.find(I);
     if (It == WorklistMap.end())
diff --git a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
index 5da9e86b20761..49842e5fd65da 100644
--- a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
@@ -110,11 +110,6 @@ Combiner::Combiner(MachineFunction &MF, CombinerInfo &CInfo,
   if (CSEInfo)
     B.setCSEInfo(CSEInfo);
 
-  // Setup observer.
-  ObserverWrapper->addObserver(WLObserver.get());
-  if (CSEInfo)
-    ObserverWrapper->addObserver(CSEInfo);
-
   B.setChangeObserver(*ObserverWrapper);
 }
 
@@ -147,6 +142,9 @@ bool Combiner::combineMachineInstrs() {
     LLVM_DEBUG(dbgs() << "\n\nCombiner iteration #" << Iteration << '\n');
 
     WorkList.clear();
+    ObserverWrapper->clearObservers();
+    if (CSEInfo)
+      ObserverWrapper->addObserver(CSEInfo);
 
     // Collect all instructions. Do a post order traversal for basic blocks and
     // insert with list bottom up, so while we pop_back_val, we'll traverse top
@@ -168,6 +166,9 @@ bool Combiner::combineMachineInstrs() {
       }
     }
     WorkList.finalize();
+
+    // Only notify WLObserver during actual combines
+    ObserverWrapper->addObserver(WLObserver.get());
     // Main Loop. Process the instructions here.
     while (!WorkList.empty()) {
       MachineInstr *CurrInst = WorkList.pop_back_val();

[tobias-stadler]

Sorry, I have no idea how that happened, I just ran spr land as normal.
Edit: I have looked at the spr source code. This seems like a bug with how [spr] landed version merge commit is created.