[globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate

Summary:
This allows us to register it with the MachineFunction delegate and be
notified automatically about erasure and creation of instructions. However,
we still need explicit notification for modifications such as those caused
by setReg() or replaceRegWith().

There is a catch with this though. The notification for creation is
delivered before any operands can be added. While appropriate for
scheduling combiner work. This is unfortunate for debug output since an
opcode by itself doesn't provide sufficient information on what happened.
As a result, the work list remembers the instructions (when debug output is
requested) and emits a more complete dump later.

Another nit is that the MachineFunction::Delegate provides const pointers
which is inconvenient since we want to use it to schedule future
modification. To resolve this GISelWorkList now has an optional pointer to
the MachineFunction which describes the scope of the work it is permitted
to schedule. If a given MachineInstr* is in this function then it is
permitted to schedule work to be performed on the MachineInstr's. An
alternative to this would be to remove the const from the
MachineFunction::Delegate interface, however delegates are not permitted
to modify the MachineInstr's they receive.

In addition to this, the observer has three interface changes.
* erasedInstr() is now erasingInstr() to indicate it is about to be erased
  but still exists at the moment.
* changingInstr() and changedInstr() have been added to report changes
  before and after they are made. This allows us to trace the changes
  in the debug output.
* As a convenience changingAllUsesOfReg() and
  finishedChangingAllUsesOfReg() will report changingInstr() and
  changedInstr() for each use of a given register. This is primarily useful
  for changes caused by MachineRegisterInfo::replaceRegWith()

With this in place, both combine rules have been updated to report their
changes to the observer.

Finally, make some cosmetic changes to the debug output and make Combiner
and CombinerHelp

Reviewers: aditya_nandakumar, bogner, volkan, rtereshin, javed.absar

Reviewed By: aditya_nandakumar

Subscribers: mgorny, rovka, kristof.beyls, llvm-commits

Differential Revision: https://reviews.llvm.org/D52947

llvm-svn: 349167

Author: dsandersllvm

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h

@@ -1,4 +1,4 @@
-//== llvm/CodeGen/GlobalISel/CombinerHelper.h -------------- -*- C++ -*-==//
+//===-- llvm/CodeGen/GlobalISel/CombinerHelper.h --------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -24,17 +24,24 @@ class GISelChangeObserver;
 class MachineIRBuilder;
 class MachineRegisterInfo;
 class MachineInstr;
+class MachineOperand;
 
 class CombinerHelper {
   MachineIRBuilder &Builder;
   MachineRegisterInfo &MRI;
   GISelChangeObserver &Observer;
 
-  void scheduleForVisit(MachineInstr &MI);
-
 public:
   CombinerHelper(GISelChangeObserver &Observer, MachineIRBuilder &B);
 
+  /// MachineRegisterInfo::replaceRegWith() and inform the observer of the changes
+  void replaceRegWith(MachineRegisterInfo &MRI, unsigned FromReg, unsigned ToReg) const;
+
+  /// Replace a single register operand with a new register and inform the
+  /// observer of the changes.
+  void replaceRegOpWith(MachineRegisterInfo &MRI, MachineOperand &FromRegOp,
+                        unsigned ToReg) const;
+
   /// If \p MI is COPY, try to combine it.
   /// Returns true if MI changed.
   bool tryCombineCopy(MachineInstr &MI);

llvm/include/llvm/CodeGen/GlobalISel/CombinerInfo.h

@@ -43,6 +43,17 @@ class CombinerInfo {
   /// illegal ops that are created.
   bool LegalizeIllegalOps; // TODO: Make use of this.
   const LegalizerInfo *LInfo;
+
+  /// Attempt to combine instructions using MI as the root.
+  ///
+  /// Use Observer to report the creation, modification, and erasure of
+  /// instructions. GISelChangeObserver will automatically report certain
+  /// kinds of operations. These operations are:
+  /// * Instructions that are newly inserted into the MachineFunction
+  /// * Instructions that are erased from the MachineFunction.
+  ///
+  /// However, it is important to report instruction modification and this is
+  /// not automatic.
   virtual bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                        MachineIRBuilder &B) const = 0;
 };

llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h

@@ -1,5 +1,4 @@
-//== ----- llvm/CodeGen/GlobalISel/GISelChangeObserver.h ---------------------
-//== //
+//===----- llvm/CodeGen/GlobalISel/GISelChangeObserver.h ------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -15,25 +14,41 @@
 #ifndef LLVM_CODEGEN_GLOBALISEL_GISELCHANGEOBSERVER_H
 #define LLVM_CODEGEN_GLOBALISEL_GISELCHANGEOBSERVER_H
 
+#include "llvm/ADT/SmallPtrSet.h"
+
 namespace llvm {
+class MachineInstr;
+class MachineRegisterInfo;
+
 /// 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 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 {
+  SmallPtrSet<MachineInstr *, 4> ChangingAllUsesOfReg;
+
 public:
   virtual ~GISelChangeObserver() {}
 
   /// An instruction is about to be erased.
-  virtual void erasingInstr(MachineInstr &MI) = 0;
+  virtual void erasingInstr(const MachineInstr &MI) = 0;
   /// An instruction was created and inserted into the function.
-  virtual void createdInstr(MachineInstr &MI) = 0;
+  virtual void createdInstr(const MachineInstr &MI) = 0;
   /// This instruction is about to be mutated in some way.
-  virtual void changingInstr(MachineInstr &MI) = 0;
+  virtual void changingInstr(const MachineInstr &MI) = 0;
   /// This instruction was mutated in some way.
-  virtual void changedInstr(MachineInstr &MI) = 0;
+  virtual void changedInstr(const MachineInstr &MI) = 0;
+
+  /// All the instructions using the given register are being changed.
+  /// For convenience, finishedChangingAllUsesOfReg() will report the completion
+  /// of the changes. The use list may change between this call and
+  /// finishedChangingAllUsesOfReg().
+  void changingAllUsesOfReg(const MachineRegisterInfo &MRI, unsigned Reg);
+  /// All instructions reported as changing by changingAllUsesOfReg() have
+  /// finished being changed.
+  void finishedChangingAllUsesOfReg();
+
 };
 
 } // namespace llvm

llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h

@@ -18,32 +18,60 @@ namespace llvm {
 
 class MachineInstr;
 
-// Worklist which mostly works similar to InstCombineWorkList, but on MachineInstrs.
-// The main difference with something like a SetVector is that erasing an element doesn't
-// move all elements over one place - instead just nulls out the element of the vector.
-// FIXME: Does it make sense to factor out common code with the instcombinerWorkList?
+// Worklist which mostly works similar to InstCombineWorkList, but on
+// MachineInstrs. The main difference with something like a SetVector is that
+// erasing an element doesn't move all elements over one place - instead just
+// nulls out the element of the vector.
+//
+// This worklist operates on instructions within a particular function. This is
+// important for acquiring the rights to modify/replace instructions a
+// GISelChangeObserver reports as the observer doesn't have the right to make
+// changes to the instructions it sees so we use our access to the
+// MachineFunction to establish that it's ok to add a given instruction to the
+// worklist.
+//
+// FIXME: Does it make sense to factor out common code with the
+// instcombinerWorkList?
 template<unsigned N>
 class GISelWorkList {
-  SmallVector<MachineInstr*, N> Worklist;
-  DenseMap<MachineInstr*, unsigned> WorklistMap;
+  MachineFunction *MF;
+  SmallVector<MachineInstr *, N> Worklist;
+  DenseMap<MachineInstr *, unsigned> WorklistMap;
 
 public:
-  GISelWorkList() = default;
+  GISelWorkList(MachineFunction *MF) : MF(MF) {}
 
   bool empty() const { return WorklistMap.empty(); }
 
   unsigned size() const { return WorklistMap.size(); }
 
-  /// Add - Add the specified instruction to the worklist if it isn't already
-  /// in it.
+  /// Add the specified instruction to the worklist if it isn't already in it.
   void insert(MachineInstr *I) {
-    if (WorklistMap.try_emplace(I, Worklist.size()).second) {
-      Worklist.push_back(I);
+    // It would be safe to add this instruction to the worklist regardless but
+    // for consistency with the const version, check that the instruction we're
+    // adding would have been accepted if we were given a const pointer instead.
+    insert(const_cast<const MachineInstr *>(I));
+  }
+
+  void insert(const MachineInstr *I) {
+    // Confirm we'd be able to find the non-const pointer we want to schedule if
+    // we wanted to. We have the right to schedule work that may modify any
+    // instruction in MF.
+    assert(I->getParent() && "Expected parent BB");
+    assert(I->getParent()->getParent() && "Expected parent function");
+    assert((!MF || I->getParent()->getParent() == MF) &&
+           "Expected parent function to be current function or not given");
+
+    // But don't actually do the search since we can derive it from the const
+    // pointer.
+    MachineInstr *NonConstI = const_cast<MachineInstr *>(I);
+    if (WorklistMap.try_emplace(NonConstI, Worklist.size()).second) {
+      Worklist.push_back(NonConstI);
     }
   }
 
-  /// Remove - remove I from the worklist if it exists.
-  void remove(MachineInstr *I) {
+  /// Remove I from the worklist if it exists.
+  void remove(const MachineInstr *I) {
     auto It = WorklistMap.find(I);
     if (It == WorklistMap.end()) return; // Not in worklist.
 

llvm/lib/CodeGen/GlobalISel/CMakeLists.txt

@@ -3,6 +3,7 @@ add_llvm_library(LLVMGlobalISel
         GlobalISel.cpp
         Combiner.cpp
         CombinerHelper.cpp
+        GISelChangeObserver.cpp
         IRTranslator.cpp
         InstructionSelect.cpp
         InstructionSelector.cpp

llvm/lib/CodeGen/GlobalISel/Combiner.cpp

@@ -1,4 +1,4 @@
-//===-- lib/CodeGen/GlobalISel/GICombiner.cpp -----------------------===//
+//===-- lib/CodeGen/GlobalISel/Combiner.cpp -------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -29,36 +29,62 @@ using namespace llvm;
 namespace {
 /// This class acts as the glue the joins the CombinerHelper to the overall
 /// Combine algorithm. The CombinerHelper is intended to report the
-/// modifications it makes to the MIR to the CombinerChangeObserver and the
+/// modifications it makes to the MIR to the GISelChangeObserver and the
 /// observer subclass will act on these events. In this case, instruction
 /// erasure will cancel any future visits to the erased instruction and
 /// instruction creation will schedule that instruction for a future visit.
 /// Other Combiner implementations may require more complex behaviour from
-/// their CombinerChangeObserver subclass.
-class WorkListMaintainer : public GISelChangeObserver {
+/// their GISelChangeObserver subclass.
+class WorkListMaintainer : public GISelChangeObserver,
+                           public MachineFunction::Delegate {
   using WorkListTy = GISelWorkList<512>;
+  MachineFunction &MF;
   WorkListTy &WorkList;
+  /// The instructions that have been created but we want to report once they
+  /// have their operands. This is only maintained if debug output is requested.
+  SmallPtrSet<const MachineInstr *, 4> CreatedInstrs;
 
 public:
-  WorkListMaintainer(WorkListTy &WorkList) : WorkList(WorkList) {}
-  virtual ~WorkListMaintainer() {}
+  WorkListMaintainer(MachineFunction &MF, WorkListTy &WorkList)
+      : GISelChangeObserver(), MF(MF), WorkList(WorkList) {
+    MF.setDelegate(this);
+  }
+  virtual ~WorkListMaintainer() {
+    MF.resetDelegate(this);
+  }
 
-  void erasingInstr(MachineInstr &MI) override {
+  void erasingInstr(const MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Erased: " << MI << "\n");
     WorkList.remove(&MI);
   }
-  void createdInstr(MachineInstr &MI) override {
-    LLVM_DEBUG(dbgs() << "Created: " << MI << "\n");
+  void createdInstr(const MachineInstr &MI) override {
+    LLVM_DEBUG(dbgs() << "Creating: " << MI << "\n");
     WorkList.insert(&MI);
+    LLVM_DEBUG(CreatedInstrs.insert(&MI));
   }
-  void changingInstr(MachineInstr &MI) override {
+  void changingInstr(const MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Changing: " << MI << "\n");
-    WorkList.remove(&MI);
+    WorkList.insert(&MI);
   }
-  // Currently changed conservatively assumes erased.
-  void changedInstr(MachineInstr &MI) override {
+  void changedInstr(const MachineInstr &MI) override {
     LLVM_DEBUG(dbgs() << "Changed: " << MI << "\n");
-    WorkList.remove(&MI);
+    WorkList.insert(&MI);
+  }
+
+  void reportFullyCreatedInstrs() {
+    LLVM_DEBUG(for (const auto *MI
+                    : CreatedInstrs) {
+      dbgs() << "Created: ";
+      MI->print(dbgs());
+    });
+    LLVM_DEBUG(CreatedInstrs.clear());
+  }
+
+  void MF_HandleInsertion(const MachineInstr &MI) override {
+    createdInstr(MI);
+  }
+  void MF_HandleRemoval(const MachineInstr &MI) override {
+    erasingInstr(MI);
   }
 };
 }
@@ -90,8 +116,8 @@ bool Combiner::combineMachineInstrs(MachineFunction &MF) {
     // insert with list bottom up, so while we pop_back_val, we'll traverse top
     // down RPOT.
     Changed = false;
-    GISelWorkList<512> WorkList;
-    WorkListMaintainer Observer(WorkList);
+    GISelWorkList<512> WorkList(&MF);
+    WorkListMaintainer Observer(MF, WorkList);
     for (MachineBasicBlock *MBB : post_order(&MF)) {
       if (MBB->empty())
         continue;
@@ -110,8 +136,9 @@ bool Combiner::combineMachineInstrs(MachineFunction &MF) {
     // Main Loop. Process the instructions here.
     while (!WorkList.empty()) {
       MachineInstr *CurrInst = WorkList.pop_back_val();
-      LLVM_DEBUG(dbgs() << "Try combining " << *CurrInst << "\n";);
+      LLVM_DEBUG(dbgs() << "\nTry combining " << *CurrInst;);
       Changed |= CInfo.combine(Observer, *CurrInst, Builder);
+      Observer.reportFullyCreatedInstrs();
     }
     MFChanged |= Changed;
   } while (Changed);