[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

1 files changed, 34 insertions, 10 deletions

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 6018c59da77..b1c5670a6de 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -1,4 +1,4 @@
-//== ---lib/CodeGen/GlobalISel/GICombinerHelper.cpp --------------------- == //
+//===-- lib/CodeGen/GlobalISel/GICombinerHelper.cpp -----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -15,7 +15,7 @@
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 
-#define DEBUG_TYPE "gi-combine"
+#define DEBUG_TYPE "gi-combiner"
 
 using namespace llvm;
 
@@ -23,8 +23,27 @@ CombinerHelper::CombinerHelper(GISelChangeObserver &Observer,
                                MachineIRBuilder &B)
     : Builder(B), MRI(Builder.getMF().getRegInfo()), Observer(Observer) {}
 
-void CombinerHelper::scheduleForVisit(MachineInstr &MI) {
-  Observer.createdInstr(MI);
+void CombinerHelper::replaceRegWith(MachineRegisterInfo &MRI, unsigned FromReg,
+                                    unsigned ToReg) const {
+  Observer.changingAllUsesOfReg(MRI, FromReg);
+
+  if (MRI.constrainRegAttrs(ToReg, FromReg))
+    MRI.replaceRegWith(FromReg, ToReg);
+  else
+    Builder.buildCopy(ToReg, FromReg);
+
+  Observer.finishedChangingAllUsesOfReg();
+}
+
+void CombinerHelper::replaceRegOpWith(MachineRegisterInfo &MRI,
+                                      MachineOperand &FromRegOp,
+                                      unsigned ToReg) const {
+  assert(FromRegOp.getParent() && "Expected an operand in an MI");
+  Observer.changingInstr(*FromRegOp.getParent());
+
+  FromRegOp.setReg(ToReg);
+
+  Observer.changedInstr(*FromRegOp.getParent());
 }
 
 bool CombinerHelper::tryCombineCopy(MachineInstr &MI) {
@@ -38,7 +57,7 @@ bool CombinerHelper::tryCombineCopy(MachineInstr &MI) {
   // a(sx) = COPY b(sx) -> Replace all uses of a with b.
   if (DstTy.isValid() && SrcTy.isValid() && DstTy == SrcTy) {
     MI.eraseFromParent();
-    MRI.replaceRegWith(DstReg, SrcReg);
+    replaceRegWith(MRI, DstReg, SrcReg);
     return true;
   }
   return false;
@@ -191,8 +210,11 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
   // type since by definition the result of an extend is larger.
   assert(Preferred.Ty != LoadValueTy && "Extending to same type?");
 
+  LLVM_DEBUG(dbgs() << "Preferred use is: " << *Preferred.MI);
+
   // Rewrite the load to the chosen extending load.
   unsigned ChosenDstReg = Preferred.MI->getOperand(0).getReg();
+  Observer.changingInstr(MI);
   MI.setDesc(
       Builder.getTII().get(Preferred.ExtendOpcode == TargetOpcode::G_SEXT
                                ? TargetOpcode::G_SEXTLOAD
@@ -211,7 +233,7 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
     if (UseMI->getOpcode() == Preferred.ExtendOpcode ||
         UseMI->getOpcode() == TargetOpcode::G_ANYEXT) {
       unsigned UseDstReg = UseMI->getOperand(0).getReg();
-      unsigned UseSrcReg = UseMI->getOperand(1).getReg();
+      MachineOperand &UseSrcMO = UseMI->getOperand(1);
       const LLT &UseDstTy = MRI.getType(UseDstReg);
       if (UseDstReg != ChosenDstReg) {
         if (Preferred.Ty == UseDstTy) {
@@ -224,7 +246,7 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
           // rewrites to:
           //    %2:_(s32) = G_SEXTLOAD ...
           //    ... = ... %2(s32)
-          MRI.replaceRegWith(UseDstReg, ChosenDstReg);
+          replaceRegWith(MRI, UseDstReg, ChosenDstReg);
           ScheduleForErase.push_back(UseMO.getParent());
         } else if (Preferred.Ty.getSizeInBits() < UseDstTy.getSizeInBits()) {
           // If the preferred size is smaller, then keep the extend but extend
@@ -237,7 +259,7 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
           //    %2:_(s32) = G_SEXTLOAD ...
           //    %3:_(s64) = G_ANYEXT %2:_(s32)
           //    ... = ... %3(s64)
-          MRI.replaceRegWith(UseSrcReg, ChosenDstReg);
+          replaceRegOpWith(MRI, UseSrcMO, ChosenDstReg);
         } else {
           // If the preferred size is large, then insert a truncate. For
           // example:
@@ -284,7 +306,9 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
 
     MachineInstr *PreviouslyEmitted = EmittedInsns.lookup(InsertIntoBB);
     if (PreviouslyEmitted) {
+      Observer.changingInstr(*UseMO->getParent());
       UseMO->setReg(PreviouslyEmitted->getOperand(0).getReg());
+      Observer.changedInstr(*UseMO->getParent());
       continue;
     }
 
@@ -292,14 +316,14 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
     unsigned NewDstReg = MRI.cloneVirtualRegister(MI.getOperand(0).getReg());
     MachineInstr *NewMI = Builder.buildTrunc(NewDstReg, ChosenDstReg);
     EmittedInsns[InsertIntoBB] = NewMI;
-    UseMO->setReg(NewDstReg);
-    Observer.createdInstr(*NewMI);
+    replaceRegOpWith(MRI, *UseMO, NewDstReg);
   }
   for (auto &EraseMI : ScheduleForErase) {
     Observer.erasingInstr(*EraseMI);
     EraseMI->eraseFromParent();
   }
   MI.getOperand(0).setReg(ChosenDstReg);
+  Observer.changedInstr(MI);
 
   return true;
 }