[llvm] InstructionSelect: Use GISelChangeObserver instead of MachineFunction::Delegate (PR #105725)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-globalisel

Author: Daniel Sanders (dsandersllvm)

<details>
<summary>Changes</summary>

The main difference is that it's possible for multiple change observers to be installed at the same time whereas there can only be one MachineFunction delegate installed. This allows downstream targets to continue to use observers to recursively select. The target in question was selecting a gMIR instruction to a machine instruction plus some gMIR around it and relying on observers to ensure it correctly selected any gMIR it created before returning to the main loop.

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


4 Files Affected:

- (modified) llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h (+15) 
- (modified) llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h (+4) 
- (modified) llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp (+10) 
- (modified) llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp (+11-4) 


``````````diff
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
index 7ec5dac9a6ebaf..214777ca9082d8 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
@@ -138,5 +138,20 @@ class RAIIMFObsDelInstaller {
   ~RAIIMFObsDelInstaller() = default;
 };
 
+/// A simple RAII based Observer installer.
+/// Use this in a scope to install the Observer to the MachineFunction and reset
+/// it at the end of the scope.
+class RAIITemporaryObserverInstaller {
+public:
+  RAIITemporaryObserverInstaller(GISelObserverWrapper &Observers,
+                                 GISelChangeObserver &TemporaryObserver);
+  ~RAIITemporaryObserverInstaller();
+
+private:
+  GISelObserverWrapper &Observers;
+  GISelChangeObserver &TemporaryObserver;
+};
+
+
 } // namespace llvm
 #endif
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
index aaba56ee11251c..ae75f2593aeffe 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
@@ -16,6 +16,8 @@
 #include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
 
 namespace llvm {
+class GISelObserverWrapper;
+
 class InstructionSelector : public GIMatchTableExecutor {
 public:
   virtual ~InstructionSelector();
@@ -36,6 +38,8 @@ class InstructionSelector : public GIMatchTableExecutor {
   const TargetPassConfig *TPC = nullptr;
 
   MachineOptimizationRemarkEmitter *MORE = nullptr;
+
+  GISelObserverWrapper *AllObservers = nullptr;
 };
 } // namespace llvm
 
diff --git a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
index 59f4d60a41d80d..836d54fa989d78 100644
--- a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
@@ -46,3 +46,13 @@ RAIIMFObserverInstaller::RAIIMFObserverInstaller(MachineFunction &MF,
 }
 
 RAIIMFObserverInstaller::~RAIIMFObserverInstaller() { MF.setObserver(nullptr); }
+
+RAIITemporaryObserverInstaller::RAIITemporaryObserverInstaller(
+    GISelObserverWrapper &Observers, GISelChangeObserver &TemporaryObserver)
+    : Observers(Observers), TemporaryObserver(TemporaryObserver) {
+  Observers.addObserver(&TemporaryObserver);
+}
+
+RAIITemporaryObserverInstaller::~RAIITemporaryObserverInstaller() {
+  Observers.removeObserver(&TemporaryObserver);
+}
diff --git a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
index 8c0bb85fd0771c..f89a0e84bcd420 100644
--- a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
@@ -76,18 +76,21 @@ InstructionSelect::InstructionSelect(CodeGenOptLevel OL, char &PassID)
 /// deletion of arbitrary instructions, we detect this case and continue
 /// selection with the predecessor of the deleted instruction.
 class InstructionSelect::MIIteratorMaintainer
-    : public MachineFunction::Delegate {
+    : public GISelChangeObserver {
 #ifndef NDEBUG
   SmallSetVector<const MachineInstr *, 32> CreatedInstrs;
 #endif
 public:
   MachineBasicBlock::reverse_iterator MII;
 
-  void MF_HandleInsertion(MachineInstr &MI) override {
+  void changingInstr(MachineInstr &MI) override {}
+  void changedInstr(MachineInstr &MI) override {}
+
+  void createdInstr(MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Creating:  " << MI; CreatedInstrs.insert(&MI));
   }
 
-  void MF_HandleRemoval(MachineInstr &MI) override {
+  void erasingInstr(MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Erasing:   " << MI; CreatedInstrs.remove(&MI));
     if (MII.getInstrIterator().getNodePtr() == &MI) {
       // If the iterator points to the MI that will be erased (i.e. the MI prior
@@ -190,8 +193,11 @@ bool InstructionSelect::selectMachineFunction(MachineFunction &MF) {
     // GISelChangeObserver, because we do not want notifications about changed
     // instructions. This prevents significant compile-time regressions from
     // e.g. constrainOperandRegClass().
+    GISelObserverWrapper AllObservers;
     MIIteratorMaintainer MIIMaintainer;
-    RAIIDelegateInstaller DelInstaller(MF, &MIIMaintainer);
+    AllObservers.addObserver(&MIIMaintainer);
+    RAIIDelegateInstaller DelInstaller(MF, &AllObservers);
+    ISel->AllObservers = &AllObservers;
 
     for (MachineBasicBlock *MBB : post_order(&MF)) {
       ISel->CurMBB = MBB;
@@ -215,6 +221,7 @@ bool InstructionSelect::selectMachineFunction(MachineFunction &MF) {
         LLVM_DEBUG(MIIMaintainer.reportFullyCreatedInstrs());
       }
     }
+    AllObservers.removeObserver(&MIIMaintainer);
   }
 
   for (MachineBasicBlock &MBB : MF) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/105725