[llvm] 0bf5846 - InstructionSelect: Use GISelChangeObserver instead of MachineFunction::Delegate (#105725)

[via llvm-commits]

Author: Daniel Sanders
Date: 2024-08-23T09:43:36-07:00
New Revision: 0bf5846553412978d30b84f06c6b6183890ab8e5

URL: https://github.com/llvm/llvm-project/commit/0bf5846553412978d30b84f06c6b6183890ab8e5
DIFF: https://github.com/llvm/llvm-project/commit/0bf5846553412978d30b84f06c6b6183890ab8e5.diff

LOG: InstructionSelect: Use GISelChangeObserver instead of MachineFunction::Delegate (#105725)

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.

Added: 
    

Modified: 
    llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
    llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
    llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
    llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
index 7ec5dac9a6ebaf..1167d51e88b71c 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
@@ -138,5 +138,19 @@ 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..fa9ab9fd760515 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,11 @@ class InstructionSelector : public GIMatchTableExecutor {
   const TargetPassConfig *TPC = nullptr;
 
   MachineOptimizationRemarkEmitter *MORE = nullptr;
+
+  /// Note: InstructionSelect does not track changed instructions.
+  /// changingInstr() and changedInstr() will never be called on these
+  /// observers.
+  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..9444ff518ca9cb 100644
--- a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
@@ -75,19 +75,25 @@ InstructionSelect::InstructionSelect(CodeGenOptLevel OL, char &PassID)
 /// a non-obvious limitation for selector implementers. Therefore, to allow
 /// deletion of arbitrary instructions, we detect this case and continue
 /// selection with the predecessor of the deleted instruction.
-class InstructionSelect::MIIteratorMaintainer
-    : public MachineFunction::Delegate {
+class InstructionSelect::MIIteratorMaintainer : 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 {
+    llvm_unreachable("InstructionSelect does not track changed instructions!");
+  }
+  void changedInstr(MachineInstr &MI) override {
+    llvm_unreachable("InstructionSelect does not track changed instructions!");
+  }
+
+  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 +196,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;