[GlobalISel] Refactor Combiner API

Remove CodeGen leftovers from the old combiner backend and adapt the API to fit the new backend better.
It's now quite a bit closer to how InstructionSelector works.

- `CombinerInfo` is now a simple "options" struct.
- `Combiner` is now the base class of all TableGen'd combiner implementation.
    - Many fields have been moved from derived classes into that class.
    - It has been refactored to create & own the Observer and Builder.
- `tryCombineAll` TableGen'd method can now be renamed, which allows targets to implement the actual `tryCombineAll` call manually and do whatever they want to do before/after it.

Note: `CombinerHelper` needs to be mutable because none of its methods are const. This can be revisited later.

Depends on D158710

Reviewed By: aemerson, dsanders

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

Author: Pierre-vh

llvm/include/llvm/CodeGen/GlobalISel/Combiner.h

@@ -6,38 +6,70 @@
 //
 //===----------------------------------------------------------------------===//
 /// \file
-/// This contains common code to drive combines. Combiner Passes will need to
-/// setup a CombinerInfo and call combineMachineFunction.
+/// This contains the base class for all Combiners generated by TableGen.
+/// Backends need to create class that inherits from "Combiner" and put all of
+/// the TableGen-erated code in there, as it implements the virtual functions.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_GLOBALISEL_COMBINER_H
 #define LLVM_CODEGEN_GLOBALISEL_COMBINER_H
 
+#include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
 
 namespace llvm {
 class MachineRegisterInfo;
-class CombinerInfo;
+struct CombinerInfo;
 class GISelCSEInfo;
 class TargetPassConfig;
 class MachineFunction;
+class MachineIRBuilder;
+
+/// Combiner implementation. This is per-function, so passes need to recreate
+/// one of these each time they enter a new function.
+///
+/// TODO: Is it worth making this module-wide?
+class Combiner : public GIMatchTableExecutor {
+private:
+  class WorkListMaintainer;
+  GISelWorkList<512> WorkList;
+
+  // We have a little hack here where keep the owned pointers private, and only
+  // expose a reference. This has two purposes:
+  //  - Avoid derived classes messing with those pointers.
+  //  - Keep the API consistent. CInfo, MF, MRI, etc. are all accessed as
+  //  references. Accessing Observer/B as pointers unnecessarily leaks
+  //  implementation details into derived classes.
+  std::unique_ptr<MachineIRBuilder> Builder;
+  std::unique_ptr<WorkListMaintainer> WLObserver;
+  std::unique_ptr<GISelObserverWrapper> ObserverWrapper;
+
+  bool HasSetupMF = false;
 
-class Combiner {
 public:
-  Combiner(CombinerInfo &CombinerInfo, const TargetPassConfig *TPC);
+  /// If CSEInfo is not null, then the Combiner will use CSEInfo as the observer
+  /// and also create a CSEMIRBuilder. Pass nullptr if CSE is not needed.
+  Combiner(MachineFunction &MF, CombinerInfo &CInfo,
+           const TargetPassConfig *TPC, GISelKnownBits *KB,
+           GISelCSEInfo *CSEInfo = nullptr);
+  virtual ~Combiner();
+
+  virtual bool tryCombineAll(MachineInstr &I) const = 0;
 
-  /// If CSEInfo is not null, then the Combiner will setup observer for
-  /// CSEInfo and instantiate a CSEMIRBuilder. Pass nullptr if CSE is not
-  /// needed.
-  bool combineMachineInstrs(MachineFunction &MF, GISelCSEInfo *CSEInfo);
+  bool combineMachineInstrs();
 
 protected:
   CombinerInfo &CInfo;
+  GISelChangeObserver &Observer;
+  MachineIRBuilder &B;
+  MachineFunction &MF;
+  MachineRegisterInfo &MRI;
+  GISelKnownBits *KB;
 
-  MachineRegisterInfo *MRI = nullptr;
   const TargetPassConfig *TPC;
-  std::unique_ptr<MachineIRBuilder> Builder;
+  GISelCSEInfo *CSEInfo;
 };
 
 } // End namespace llvm.

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

@@ -6,7 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 /// \file
-/// Interface for Targets to specify which operations are combined how and when.
+/// Option class for Targets to specify which operations are combined how and
+/// when.
 ///
 //===----------------------------------------------------------------------===//
 
@@ -16,15 +17,11 @@
 #include <cassert>
 namespace llvm {
 
-class GISelChangeObserver;
 class LegalizerInfo;
-class MachineInstr;
-class MachineIRBuilder;
 
 // Contains information relevant to enabling/disabling various combines for a
 // pass.
-class CombinerInfo {
-public:
+struct CombinerInfo {
   CombinerInfo(bool AllowIllegalOps, bool ShouldLegalizeIllegal,
                const LegalizerInfo *LInfo, bool OptEnabled, bool OptSize,
                bool MinSize)
@@ -52,19 +49,6 @@ class CombinerInfo {
   bool EnableOptSize;
   /// Whether we're optimizing for minsize (-Oz).
   bool EnableMinSize;
-
-  /// 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;
 };
 } // namespace llvm
 

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h

@@ -463,9 +463,7 @@ class GIMatchTableExecutor {
   // For some predicates, we need to track the current MBB.
   MachineBasicBlock *CurMBB = nullptr;
 
-  virtual void setupGeneratedPerFunctionState(MachineFunction &MF) {
-    llvm_unreachable("TableGen should have emitted implementation");
-  }
+  virtual void setupGeneratedPerFunctionState(MachineFunction &MF) = 0;
 
   /// Setup per-MF executor state.
   virtual void setupMF(MachineFunction &mf, GISelKnownBits *kb,

llvm/include/llvm/Target/GlobalISel/Combine.td

@@ -37,6 +37,9 @@ class GICombinerHelper<string classname, list<GICombine> rules>
     : GICombineGroup<rules> {
   // The class name to use in the generated output.
   string Classname = classname;
+  // Combiners can use this so they're free to define tryCombineAll themselves
+  // and do extra work before/after calling the TableGen-erated code.
+  string CombineAllMethodName = "tryCombineAll";
   // The name of a run-time compiler option that will be generated to disable
   // specific rules within this combiner.
   string DisableRuleOption = ?;

llvm/lib/CodeGen/GlobalISel/Combiner.cpp

@@ -39,7 +39,6 @@ cl::OptionCategory GICombinerOptionCategory(
 );
 } // end 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 GISelChangeObserver and the
@@ -48,7 +47,7 @@ namespace {
 /// instruction creation will schedule that instruction for a future visit.
 /// Other Combiner implementations may require more complex behaviour from
 /// their GISelChangeObserver subclass.
-class WorkListMaintainer : public GISelChangeObserver {
+class Combiner::WorkListMaintainer : public GISelChangeObserver {
   using WorkListTy = GISelWorkList<512>;
   WorkListTy &WorkList;
   /// The instructions that have been created but we want to report once they
@@ -88,54 +87,70 @@ class WorkListMaintainer : public GISelChangeObserver {
     LLVM_DEBUG(CreatedInstrs.clear());
   }
 };
-}
 
-Combiner::Combiner(CombinerInfo &Info, const TargetPassConfig *TPC)
-    : CInfo(Info), TPC(TPC) {
+Combiner::Combiner(MachineFunction &MF, CombinerInfo &CInfo,
+                   const TargetPassConfig *TPC, GISelKnownBits *KB,
+                   GISelCSEInfo *CSEInfo)
+    : Builder(CSEInfo ? std::make_unique<CSEMIRBuilder>()
+                      : std::make_unique<MachineIRBuilder>()),
+      WLObserver(std::make_unique<WorkListMaintainer>(WorkList)),
+      ObserverWrapper(std::make_unique<GISelObserverWrapper>()), CInfo(CInfo),
+      Observer(*ObserverWrapper), B(*Builder), MF(MF), MRI(MF.getRegInfo()),
+      KB(KB), TPC(TPC), CSEInfo(CSEInfo) {
   (void)this->TPC; // FIXME: Remove when used.
+
+  // Setup builder.
+  B.setMF(MF);
+  if (CSEInfo)
+    B.setCSEInfo(CSEInfo);
+
+  // Setup observer.
+  ObserverWrapper->addObserver(WLObserver.get());
+  if (CSEInfo)
+    ObserverWrapper->addObserver(CSEInfo);
+
+  B.setChangeObserver(*ObserverWrapper);
 }
 
-bool Combiner::combineMachineInstrs(MachineFunction &MF,
-                                    GISelCSEInfo *CSEInfo) {
+Combiner::~Combiner() = default;
+
+bool Combiner::combineMachineInstrs() {
   // If the ISel pipeline failed, do not bother running this pass.
   // FIXME: Should this be here or in individual combiner passes.
   if (MF.getProperties().hasProperty(
           MachineFunctionProperties::Property::FailedISel))
     return false;
 
-  Builder =
-      CSEInfo ? std::make_unique<CSEMIRBuilder>() : std::make_unique<MachineIRBuilder>();
-  MRI = &MF.getRegInfo();
-  Builder->setMF(MF);
-  if (CSEInfo)
-    Builder->setCSEInfo(CSEInfo);
+  // We can't call this in the constructor because the derived class is
+  // uninitialized at that time.
+  if (!HasSetupMF) {
+    HasSetupMF = true;
+    setupMF(MF, KB);
+  }
 
   LLVM_DEBUG(dbgs() << "Generic MI Combiner for: " << MF.getName() << '\n');
 
   MachineOptimizationRemarkEmitter MORE(MF, /*MBFI=*/nullptr);
 
   bool MFChanged = false;
   bool Changed;
-  MachineIRBuilder &B = *Builder;
 
   do {
+    WorkList.clear();
+
     // 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
     // down RPOT.
     Changed = false;
-    GISelWorkList<512> WorkList;
-    WorkListMaintainer Observer(WorkList);
-    GISelObserverWrapper WrapperObserver(&Observer);
-    if (CSEInfo)
-      WrapperObserver.addObserver(CSEInfo);
-    RAIIDelegateInstaller DelInstall(MF, &WrapperObserver);
+
+    RAIIDelegateInstaller DelInstall(MF, ObserverWrapper.get());
     for (MachineBasicBlock *MBB : post_order(&MF)) {
       for (MachineInstr &CurMI :
            llvm::make_early_inc_range(llvm::reverse(*MBB))) {
         // Erase dead insts before even adding to the list.
-        if (isTriviallyDead(CurMI, *MRI)) {
+        if (isTriviallyDead(CurMI, MRI)) {
           LLVM_DEBUG(dbgs() << CurMI << "Is dead; erasing.\n");
-          llvm::salvageDebugInfo(*MRI, CurMI);
+          llvm::salvageDebugInfo(MRI, CurMI);
           CurMI.eraseFromParent();
           continue;
         }
@@ -147,8 +162,8 @@ bool Combiner::combineMachineInstrs(MachineFunction &MF,
     while (!WorkList.empty()) {
       MachineInstr *CurrInst = WorkList.pop_back_val();
       LLVM_DEBUG(dbgs() << "\nTry combining " << *CurrInst;);
-      Changed |= CInfo.combine(WrapperObserver, *CurrInst, B);
-      Observer.reportFullyCreatedInstrs();
+      Changed |= tryCombineAll(*CurrInst);
+      WLObserver->reportFullyCreatedInstrs();
     }
     MFChanged |= Changed;
   } while (Changed);

llvm/lib/Target/AArch64/AArch64Combine.td

@@ -38,10 +38,12 @@ def AArch64PreLegalizerCombiner: GICombinerHelper<
                                       fconstant_to_constant,
                                       icmp_redundant_trunc,
                                       fold_global_offset]> {
+  let CombineAllMethodName = "tryCombineAllImpl";
 }
 
 def AArch64O0PreLegalizerCombiner: GICombinerHelper<
   "AArch64O0PreLegalizerCombinerImpl", [optnone_combines]> {
+  let CombineAllMethodName = "tryCombineAllImpl";
 }
 
 // Matchdata for combines which replace a G_SHUFFLE_VECTOR with a