[gicombiner] Unify common state for current targets into CommonTargetCombinerHelperState

[dsanders]

Summary

At this point tryCombine*() in the existing combiners takes a single MI argument.

Depends on D81889

[arsenm]

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

278–279

I've been trying to sort out the relationship between MachineIRBuilder, *Helper, and observers recently. What is the relationship between the builder and observer here? Is it expected the builder contains this observer, or is this observer expected to also be an observer in the builder?

[dsanders]

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

278–279

Helpers need to tell observers what they did so that the enclosing algorithm can maintain its state (e.g to forget erased instructions). For convenience of implementation, Helpers rely on the MachineFunction delegate to report create/erase. The Helper must report about-to-change/changed directly to the observer.

Something must inform the CSE MIR Builder about invalidated state. It doesn't matter too much whether it's the MachineFunction delegate or the MachineIRBuilder that does this so long as it always happens for every instruction create/delete (the delegate is preferred if instrs can be built without the MachineIRBuilder but otherwise it's equivalent). There was a point where we didn't have the observer that could forward to multiple observers. That could explain why MachineIRBuilder has an observer as well. IIRC, there was also a CSE MIR Builder that could only CSE things the builder made at one point.

So in summary, the algorithms observer and the LLVM_DEBUG() printing observer (which may be the same one) must:

• Given to the Helper
• If MachineIRBuilder is the only way instructions are created: Registered with either the MachineFunction delegate or the MachineIRBuilder
• Otherwise: Registered with the MachineFunction delegate

and the CSE MIR Builders observer must:

• Be registered with either the MachineFunction delegate or the MachineIRBuilder

[aditya_nandakumar]

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

278–279

I am thinking we should simplify this and make sure the MachineFunction has the observer installed before we call any hooks. Observers could then be accessed through MF.getObserver().
In other words,

1. MF always has observer installed. Automatically tracks insertions, deletions.
2. For mutations, one can get to the observer either through MF (or a forwarding call in the builder if that's convenient).

I'm not sure if there's ever a case where it makes sense to have different observers installed in the MF and the builders (that IMO will lead to confusions).

[aemerson]

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

278–279

I am thinking we should simplify this and make sure the MachineFunction has the observer installed before we call any hooks. Observers could then be accessed through MF.getObserver().
In other words,

MF always has observer installed. Automatically tracks insertions, deletions.
For mutations, one can get to the observer either through MF (or a forwarding call in the builder if that's convenient).
I'm not sure if there's ever a case where it makes sense to have different observers installed in the MF and the builders (that IMO will lead to confusions).

+1

[arsenm]

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

278–279

So in this case, the Observer here is supposed to also be present in either/or the MIRBuilder or MachineFunction, but the combiner also needs to inform this? Should there be an assert this observer is present in the builder/function?

[aditya_nandakumar]

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

278–279

I was thinking that the MF will have the observer, and the MIRBuilder does not store the observer at all (and calls to Builder.getObserver() does the equivalent of Builder.getMF().getObserver()). With this, we can stop passing in the observer to all APIs.

[arsenm]

Can you add a comment to the observer member about where it's contained

[arsenm]

bump

[arsenm]

reverse ping

[arsenm]

reverse ping

[arsenm]

Reverse ping

[arsenm]

Is this still relevant?

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

private:
 
   /// Given a non-indexed load or store instruction \p MI, find an offset that
   /// can be usefully and legally folded into it as a pre-indexing operation.
   ///
   /// \returns true if a candidate is found.
   bool findPreIndexCandidate(MachineInstr &MI, Register &Addr, Register &Base,
                              Register &Offset);
 };
+
+class CommonTargetCombinerHelperState {
+protected:
+  GISelChangeObserver &Observer;
+  MachineIRBuilder &B;
+  CombinerHelper &Helper;
+
+public:
+  CommonTargetCombinerHelperState(GISelChangeObserver &Observer,
+                                  MachineIRBuilder &B, CombinerHelper &Helper)
+      : Observer(Observer), B(B), Helper(Helper) {}
+};
+
 } // namespace llvm
 
 #endif

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

class GICombinerHelper<string classname, list<GICombine> rules>
     : GICombineGroup<rules> {
   // The class name to use in the generated output.
   string Classname = classname;
   // The name of a run-time compiler option that will be generated to disable
   // specific rules within this combiner.
   string DisableRuleOption = ?;
   // The state class to inherit from (if any). The generated helper will inherit
   // from this class and will forward arguments to its constructors.
-  string StateClass = "";
+  string StateClass = "CommonTargetCombinerHelperState";
   // Any additional arguments that should be appended to the tryCombine*().
-  list<GICombinerHelperArg> AdditionalArguments =
-      [GICombinerHelperArg<"CombinerHelper &", "Helper">];
+  list<GICombinerHelperArg> AdditionalArguments = [];
 }
 class GICombineRule<dag defs, dag match, dag apply> : GICombine {
   /// Defines the external interface of the match rule. This includes:
   /// * The names of the root nodes (requires at least one)
   /// See GIDefKind for details.
   dag Defs = defs;
 
   /// Defines the things which must be true for the pattern to match

llvm/lib/Target/AArch64/AArch64Combine.td

(match (wip_match_opcode G_FCONSTANT):$root,
          [{ return matchFConstantToConstant(*${root}, MRI); }]),
   (apply [{ applyFConstantToConstant(*${root}); }])>;
 
 def AArch64PreLegalizerCombinerHelper: GICombinerHelper<
   "AArch64GenPreLegalizerCombinerHelper", [all_combines,
                                            elide_br_by_inverting_cond,
                                            fconstant_to_constant]> {
   let DisableRuleOption = "aarch64prelegalizercombiner-disable-rule";
-  let StateClass = "AArch64PreLegalizerCombinerHelperState";
-  let AdditionalArguments = [];
 }
 
 // Matchdata for combines which replace a G_SHUFFLE_VECTOR with a
 // target-specific opcode.
 def shuffle_matchdata : GIDefMatchData<"ShuffleVectorPseudo">;
 
 def rev : GICombineRule<
   (defs root:$root, shuffle_matchdata:$matchinfo),

llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp

 };
 
 bool AArch64PostLegalizerCombinerInfo::combine(GISelChangeObserver &Observer,
                                                MachineInstr &MI,
                                                MachineIRBuilder &B) const {
   const auto *LI =
       MI.getParent()->getParent()->getSubtarget().getLegalizerInfo();
   CombinerHelper Helper(Observer, B, KB, MDT, LI);
-  AArch64GenPostLegalizerCombinerHelper Generated(GeneratedRuleCfg);
-  return Generated.tryCombineAll(Observer, MI, B, Helper);
+  AArch64GenPostLegalizerCombinerHelper Generated(GeneratedRuleCfg, Observer, B,
+                                                  Helper);
+  return Generated.tryCombineAll(MI);
 }
 
 #define AARCH64POSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_CPP
 #include "AArch64GenPostLegalizeGICombiner.inc"
 #undef AARCH64POSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_CPP
 
 class AArch64PostLegalizerCombiner : public MachineFunctionPass {
 public:

llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp

 static void applyFConstantToConstant(MachineInstr &MI) {
   assert(MI.getOpcode() == TargetOpcode::G_FCONSTANT);
   MachineIRBuilder MIB(MI);
   const APFloat &ImmValAPF = MI.getOperand(1).getFPImm()->getValueAPF();
   MIB.buildConstant(MI.getOperand(0).getReg(), ImmValAPF.bitcastToAPInt());
   MI.eraseFromParent();
 }
 
-class AArch64PreLegalizerCombinerHelperState {
-protected:
-  CombinerHelper &Helper;
-
-public:
-  AArch64PreLegalizerCombinerHelperState(CombinerHelper &Helper)
-      : Helper(Helper) {}
-};
-
 #define AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_DEPS
 #include "AArch64GenPreLegalizeGICombiner.inc"
 #undef AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_DEPS
 
 namespace {
 #define AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
 #include "AArch64GenPreLegalizeGICombiner.inc"
 #undef AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
public:
   virtual bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                        MachineIRBuilder &B) const override;
 };
 
 bool AArch64PreLegalizerCombinerInfo::combine(GISelChangeObserver &Observer,
                                               MachineInstr &MI,
                                               MachineIRBuilder &B) const {
   CombinerHelper Helper(Observer, B, KB, MDT);
-  AArch64GenPreLegalizerCombinerHelper Generated(GeneratedRuleCfg, Helper);
+  AArch64GenPreLegalizerCombinerHelper Generated(GeneratedRuleCfg, Observer, B,
+                                                 Helper);
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
     switch (MI.getIntrinsicID()) {
     case Intrinsic::memcpy:
     case Intrinsic::memmove:
     case Intrinsic::memset: {
       // If we're at -O0 set a maxlen of 32 to inline, otherwise let the other
       // heuristics decide.
       unsigned MaxLen = EnableOpt ? 0 : 32;
       // Try to inline memcpy type calls if optimizations are enabled.
       return (!EnableMinSize) ? Helper.tryCombineMemCpyFamily(MI, MaxLen)
                               : false;
     }
     default:
       break;
     }
   }
 
-  if (Generated.tryCombineAll(Observer, MI, B))
+  if (Generated.tryCombineAll(MI))
     return true;
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_CONCAT_VECTORS:
     return Helper.tryCombineConcatVectors(MI);
   case TargetOpcode::G_SHUFFLE_VECTOR:
     return Helper.tryCombineShuffleVector(MI);
   }

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

public:
   bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                MachineIRBuilder &B) const override;
 };
 
 bool AMDGPUPostLegalizerCombinerInfo::combine(GISelChangeObserver &Observer,
                                               MachineInstr &MI,
                                               MachineIRBuilder &B) const {
   CombinerHelper Helper(Observer, B, KB, MDT);
-  AMDGPUGenPostLegalizerCombinerHelper Generated(GeneratedRuleCfg);
+  AMDGPUGenPostLegalizerCombinerHelper Generated(GeneratedRuleCfg, Observer, B,
+                                                 Helper);
 
-  if (Generated.tryCombineAll(Observer, MI, B, Helper))
+  if (Generated.tryCombineAll(MI))
     return true;
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_SHL:
   case TargetOpcode::G_LSHR:
   case TargetOpcode::G_ASHR:
     // On some subtargets, 64-bit shift is a quarter rate instruction. In the
     // common case, splitting this into a move and a 32-bit shift is faster and

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

public:
   virtual bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                        MachineIRBuilder &B) const override;
 };
 
 bool AMDGPUPreLegalizerCombinerInfo::combine(GISelChangeObserver &Observer,
                                               MachineInstr &MI,
                                               MachineIRBuilder &B) const {
   CombinerHelper Helper(Observer, B, KB, MDT);
-  AMDGPUGenPreLegalizerCombinerHelper Generated(GeneratedRuleCfg);
+  AMDGPUGenPreLegalizerCombinerHelper Generated(GeneratedRuleCfg, Observer, B,
+                                                Helper);
 
-  if (Generated.tryCombineAll(Observer, MI, B, Helper))
+  if (Generated.tryCombineAll(MI))
     return true;
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_CONCAT_VECTORS:
     return Helper.tryCombineConcatVectors(MI);
   case TargetOpcode::G_SHUFFLE_VECTOR:
     return Helper.tryCombineShuffleVector(MI);
   }

llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp

public:
   bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                MachineIRBuilder &B) const override;
 };
 
 bool AMDGPURegBankCombinerInfo::combine(GISelChangeObserver &Observer,
                                               MachineInstr &MI,
                                               MachineIRBuilder &B) const {
   CombinerHelper Helper(Observer, B, KB, MDT);
-  AMDGPUGenRegBankCombinerHelper Generated(GeneratedRuleCfg);
+  AMDGPUGenRegBankCombinerHelper Generated(GeneratedRuleCfg, Observer, B,
+                                           Helper);
 
-  if (Generated.tryCombineAll(Observer, MI, B, Helper))
+  if (Generated.tryCombineAll(MI))
     return true;
 
   return false;
 }
 
 #define AMDGPUREGBANKCOMBINERHELPER_GENCOMBINERHELPER_CPP
 #include "AMDGPUGenRegBankGICombiner.inc"
 #undef AMDGPUREGBANKCOMBINERHELPER_GENCOMBINERHELPER_CPP

llvm/utils/TableGen/GICombinerEmitter.cpp

if (Leaf.isFullyTested() &&
          << "llvm_unreachable(\"Combine rule elision was incorrect\");\n"
          << Indent << "return false;\n";
     }
   }
   if (!AnyFullyTested)
     OS << Indent << "return false;\n";
 }
 
-static void emitAdditionalHelperMethodArguments(raw_ostream &OS,
-                                                Record *Combiner) {
+static void emitHelperMethodArguments(raw_ostream &OS, Record *Combiner) {
+  OS << "    MachineInstr &MI\n";
   for (Record *Arg : Combiner->getValueAsListOfDefs("AdditionalArguments"))
     OS << ",\n    " << Arg->getValueAsString("Type")
        << Arg->getValueAsString("Name");
 }
 
 void GICombinerEmitter::run(raw_ostream &OS) {
   gatherRules(Rules, Combiner->getValueAsListOfDefs("Rules"));
   if (StopAfterParse) {
OS << " {\n"
      << "\n"
      << "public:\n"
      << "  template<typename ... Args>" << getClassName() << "(const "
      << getClassName() << "RuleConfig &RuleConfig, Args &&... args) : ";
   if (!StateClass.empty())
     OS << StateClass << "(std::forward<Args>(args)...), ";
   OS << "RuleConfig(&RuleConfig) {}\n"
      << "\n"
-     << "  bool tryCombineAll(\n"
-     << "    GISelChangeObserver &Observer,\n"
-     << "    MachineInstr &MI,\n"
-     << "    MachineIRBuilder &B";
-  emitAdditionalHelperMethodArguments(OS, Combiner);
+     << "  bool tryCombineAll(\n";
+  emitHelperMethodArguments(OS, Combiner);
   OS << ") const;\n";
   OS << "};\n\n";
 
   emitNameMatcher(OS);
 
   OS << "static Optional<std::pair<uint64_t, uint64_t>> "
         "getRuleRangeForIdentifier(StringRef RuleIdentifier) {\n"
      << "  std::pair<StringRef, StringRef> RangePair = "
OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_CPP\n"
      << "    if (Enabled && !setRuleEnabled(Identifier))\n"
      << "      return false;\n"
      << "    if (!Enabled && !setRuleDisabled(Identifier))\n"
      << "      return false;\n"
      << "  }\n"
      << "  return true;\n"
      << "}\n\n";
 
-  OS << "bool " << getClassName() << "::tryCombineAll(\n"
-     << "    GISelChangeObserver &Observer,\n"
-     << "    MachineInstr &MI,\n"
-     << "    MachineIRBuilder &B";
-  emitAdditionalHelperMethodArguments(OS, Combiner);
+  OS << "bool " << getClassName() << "::tryCombineAll(\n";
+  emitHelperMethodArguments(OS, Combiner);
   OS << ") const {\n"
      << "  MachineBasicBlock *MBB = MI.getParent();\n"
      << "  MachineFunction *MF = MBB->getParent();\n"
      << "  MachineRegisterInfo &MRI = MF->getRegInfo();\n"
      << "  SmallVector<MachineInstr *, 8> MIs = { &MI };\n\n"
      << "  (void)MBB; (void)MF; (void)MRI;\n\n";
 
   OS << "  // Match data\n";