[gicombiner] Allow generated combiners to store additional members

[dsanders]

Summary

Adds the ability to add members to a generated combiner via
a State base class. In the current AArch64PreLegalizerCombiner
this is used to make Helper available without having to
provide it to every call.

As part of this, split the command line processing into a
separate object so that it still only runs once even though
the generated combiner is constructed more frequently.

Depends on D81862

[arsenm]

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

58

Should every combiner just get this by default? I would think every combiner pass would use this, and this just adds more boilerplate to every target?

[arsenm]

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

58

I guess this does let you choose your member name

[dsanders]

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

58

It also allows you to choose the type or add more context. One example of this is that downstream we're going to have a TgtHelper that contains our target specific combines and start moving the more generic parts of it upstream. I'm not currently sure whether that's as well as having Helper or if we can just change the type to our target specific one and still call it Helper.

Another example, is it allows targets to share code between different combiner phases while still allowing the phases to differ. Probably the most common form of this will be a pre/post legalizer predicate so that pre-legalize can simplify as far as possible, while post-legalize enforces additional predicates (or takes additional steps) to ensure legality is preserved.

If we were to have a target independent set for combiners I'd do it via a base class for the State class so that it's possible to remove things too. I wouldn't hardcode it because there's also the potential future uses to consider, using this in other passes (especially target specific ones) where having a CombinerHelper imposed makes less sense. At this point, each combiner that just uses the default could just have:

let StateClass = "CombinerHelperState";

(or that could be the default), those that only want to extend can subclass CombinerHelperState and provide their subclasses name, and finally those that want to omit things can provide their own class that doesn't subclass CombinerHelperState.

I don't mind adding a follow up patch that unifies the targets to a default state class

[aditya_nandakumar]

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

375

These names are getting a bit too long. Can we come up with a better naming scheme for these?

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

77

Similar with how long this is?

[dsanders]

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

375

I agree but it's written exactly once so I'm not too worried about it at the moment. It could become a problem if we start passing them around though. I might make it a class-scoped class at some point but that won't really make it any shorter in this particular definition (actually it will add :: to it).

Part of the current name is user selected so it's really ${prefix}RuleConfig. Maybe tablegen should let you pick both names (that would prevent class-scoping it but that's not much of a loss).

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

 // Declares a combiner helper class
 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 = "";
   // Any additional arguments that should be appended to the tryCombine*().
   list<GICombinerHelperArg> AdditionalArguments =
       [GICombinerHelperArg<"CombinerHelper &", "Helper">];
 }
 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.

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

 #include "AArch64GenPostLegalizeGICombiner.inc"
 #undef AARCH64POSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
 
 class AArch64PostLegalizerCombinerInfo : public CombinerInfo {
   GISelKnownBits *KB;
   MachineDominatorTree *MDT;
 
 public:
-  AArch64GenPostLegalizerCombinerHelper Generated;
+  AArch64GenPostLegalizerCombinerHelperRuleConfig GeneratedRuleCfg;
 
   AArch64PostLegalizerCombinerInfo(bool EnableOpt, bool OptSize, bool MinSize,
                                    GISelKnownBits *KB,
                                    MachineDominatorTree *MDT)
       : CombinerInfo(/*AllowIllegalOps*/ true, /*ShouldLegalizeIllegal*/ false,
                      /*LegalizerInfo*/ nullptr, EnableOpt, OptSize, MinSize),
         KB(KB), MDT(MDT) {
-    if (!Generated.parseCommandLineOption())
+    if (!GeneratedRuleCfg.parseCommandLineOption())
       report_fatal_error("Invalid rule identifier");
   }
 
   virtual bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                        MachineIRBuilder &B) const override;
 };
 
 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);
 }
 
 #define AARCH64POSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_CPP
 #include "AArch64GenPostLegalizeGICombiner.inc"
 #undef AARCH64POSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_CPP
 
 class AArch64PostLegalizerCombiner : public MachineFunctionPass {

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
 
 class AArch64PreLegalizerCombinerInfo : public CombinerInfo {
   GISelKnownBits *KB;
   MachineDominatorTree *MDT;
+  AArch64GenPreLegalizerCombinerHelperRuleConfig GeneratedRuleCfg;
 
 public:
-  AArch64GenPreLegalizerCombinerHelper Generated;
-
   AArch64PreLegalizerCombinerInfo(bool EnableOpt, bool OptSize, bool MinSize,
                                   GISelKnownBits *KB, MachineDominatorTree *MDT)
       : CombinerInfo(/*AllowIllegalOps*/ true, /*ShouldLegalizeIllegal*/ false,
                      /*LegalizerInfo*/ nullptr, EnableOpt, OptSize, MinSize),
         KB(KB), MDT(MDT) {
-    if (!Generated.parseCommandLineOption())
+    if (!GeneratedRuleCfg.parseCommandLineOption())
       report_fatal_error("Invalid rule identifier");
   }
 
   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);
 
   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, Helper))
+  if (Generated.tryCombineAll(Observer, MI, B))
     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

 #include "AMDGPUGenPostLegalizeGICombiner.inc"
 #undef AMDGPUPOSTLEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
 
 class AMDGPUPostLegalizerCombinerInfo : public CombinerInfo {
   GISelKnownBits *KB;
   MachineDominatorTree *MDT;
 
 public:
-  AMDGPUGenPostLegalizerCombinerHelper Generated;
+  AMDGPUGenPostLegalizerCombinerHelperRuleConfig GeneratedRuleCfg;
 
   AMDGPUPostLegalizerCombinerInfo(bool EnableOpt, bool OptSize, bool MinSize,
                                   const AMDGPULegalizerInfo *LI,
                                   GISelKnownBits *KB, MachineDominatorTree *MDT)
       : CombinerInfo(/*AllowIllegalOps*/ false, /*ShouldLegalizeIllegal*/ true,
                      /*LegalizerInfo*/ LI, EnableOpt, OptSize, MinSize),
         KB(KB), MDT(MDT) {
-    if (!Generated.parseCommandLineOption())
+    if (!GeneratedRuleCfg.parseCommandLineOption())
       report_fatal_error("Invalid rule identifier");
   }
 
   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);
 
   if (Generated.tryCombineAll(Observer, MI, B, Helper))
     return true;
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_SHL:
   case TargetOpcode::G_LSHR:
   case TargetOpcode::G_ASHR:

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

 #include "AMDGPUGenPreLegalizeGICombiner.inc"
 #undef AMDGPUPRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
 
 class AMDGPUPreLegalizerCombinerInfo : public CombinerInfo {
   GISelKnownBits *KB;
   MachineDominatorTree *MDT;
 
 public:
-  AMDGPUGenPreLegalizerCombinerHelper Generated;
+  AMDGPUGenPreLegalizerCombinerHelperRuleConfig GeneratedRuleCfg;
 
   AMDGPUPreLegalizerCombinerInfo(bool EnableOpt, bool OptSize, bool MinSize,
                                   GISelKnownBits *KB, MachineDominatorTree *MDT)
       : CombinerInfo(/*AllowIllegalOps*/ true, /*ShouldLegalizeIllegal*/ false,
                      /*LegalizerInfo*/ nullptr, EnableOpt, OptSize, MinSize),
         KB(KB), MDT(MDT) {
-    if (!Generated.parseCommandLineOption())
+    if (!GeneratedRuleCfg.parseCommandLineOption())
       report_fatal_error("Invalid rule identifier");
   }
 
   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);
 
   if (Generated.tryCombineAll(Observer, MI, B, Helper))
     return true;
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_CONCAT_VECTORS:
     return Helper.tryCombineConcatVectors(MI);
   case TargetOpcode::G_SHUFFLE_VECTOR:

llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp

 #include "AMDGPUGenRegBankGICombiner.inc"
 #undef AMDGPUREGBANKCOMBINERHELPER_GENCOMBINERHELPER_H
 
 class AMDGPURegBankCombinerInfo : public CombinerInfo {
   GISelKnownBits *KB;
   MachineDominatorTree *MDT;
 
 public:
-  AMDGPUGenRegBankCombinerHelper Generated;
+  AMDGPUGenRegBankCombinerHelperRuleConfig GeneratedRuleCfg;
 
   AMDGPURegBankCombinerInfo(bool EnableOpt, bool OptSize, bool MinSize,
                                   const AMDGPULegalizerInfo *LI,
                                   GISelKnownBits *KB, MachineDominatorTree *MDT)
       : CombinerInfo(/*AllowIllegalOps*/ false, /*ShouldLegalizeIllegal*/ true,
                      /*LegalizerInfo*/ LI, EnableOpt, OptSize, MinSize),
         KB(KB), MDT(MDT) {
-    if (!Generated.parseCommandLineOption())
+    if (!GeneratedRuleCfg.parseCommandLineOption())
       report_fatal_error("Invalid rule identifier");
   }
 
   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);
 
   if (Generated.tryCombineAll(Observer, MI, B, Helper))
     return true;
 
   return false;
 }
 
 #define AMDGPUREGBANKCOMBINERHELPER_GENCOMBINERHELPER_CPP

llvm/utils/TableGen/GICombinerEmitter.cpp

void GICombinerEmitter::generateCodeForTree(raw_ostream &OS,
   bool AnyFullyTested = false;
   for (const auto &Leaf : Tree.possible_leaves()) {
     OS << Indent << "// Leaf name: " << Leaf.getName() << "\n";
 
     const CombineRule *Rule = Leaf.getTargetData<CombineRule>();
     const Record &RuleDef = Rule->getDef();
 
     OS << Indent << "// Rule: " << RuleDef.getName() << "\n"
-       << Indent << "if (!isRuleDisabled(" << Rule->getID() << ")) {\n";
+       << Indent << "if (!RuleConfig->isRuleDisabled(" << Rule->getID()
+       << ")) {\n";
 
     CodeExpansions Expansions;
     for (const auto &VarBinding : Leaf.var_bindings()) {
       if (VarBinding.isInstr())
         Expansions.declare(VarBinding.getName(),
                            "MIs[" + to_string(VarBinding.getInstrID()) + "]");
       else
         Expansions.declare(VarBinding.getName(),
void GICombinerEmitter::run(raw_ostream &OS) {
   OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_DEPS\n"
      << "#include \"llvm/ADT/SparseBitVector.h\"\n"
      << "namespace llvm {\n"
      << "extern cl::OptionCategory GICombinerOptionCategory;\n"
      << "} // end namespace llvm\n"
      << "#endif // ifdef " << Name.upper() << "_GENCOMBINERHELPER_DEPS\n\n";
 
   OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_H\n"
-     << "class " << getClassName() << " {\n"
+     << "class " << getClassName() << "RuleConfig {\n"
      << "  SparseBitVector<> DisabledRules;\n"
      << "\n"
      << "public:\n"
      << "  bool parseCommandLineOption();\n"
      << "  bool isRuleDisabled(unsigned ID) const;\n"
      << "  bool setRuleDisabled(StringRef RuleIdentifier);\n"
      << "\n"
+     << "};\n"
+     << "\n"
+     << "class " << getClassName();
+  StringRef StateClass = Combiner->getValueAsString("StateClass");
+  if (!StateClass.empty())
+    OS << " : public " << StateClass;
+  OS << " {\n"
+     << " const " << getClassName() << "RuleConfig *RuleConfig;\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);
   OS << ") const;\n";
   OS << "};\n\n";
 
   emitNameMatcher(OS);
 
   OS << "bool " << getClassName()
-     << "::setRuleDisabled(StringRef RuleIdentifier) {\n"
+     << "RuleConfig::setRuleDisabled(StringRef RuleIdentifier) {\n"
      << "  std::pair<StringRef, StringRef> RangePair = "
         "RuleIdentifier.split('-');\n"
      << "  if (!RangePair.second.empty()) {\n"
      << "    const auto First = getRuleIdxForIdentifier(RangePair.first);\n"
      << "    const auto Last = getRuleIdxForIdentifier(RangePair.second);\n"
      << "    if (!First.hasValue() || !Last.hasValue())\n"
      << "      return false;\n"
      << "    if (First >= Last)\n"
      << "      report_fatal_error(\"Beginning of range should be before end of "
         "range\");\n"
      << "    for (auto I = First.getValue(); I < Last.getValue(); ++I)\n"
      << "      DisabledRules.set(I);\n"
      << "    return true;\n"
      << "  } else {\n"
      << "    const auto I = getRuleIdxForIdentifier(RangePair.first);\n"
      << "    if (!I.hasValue())\n"
      << "      return false;\n"
      << "    DisabledRules.set(I.getValue());\n"
      << "    return true;\n"
      << "  }\n"
      << "  return false;\n"
      << "}\n";
 
   OS << "bool " << getClassName()
-     << "::isRuleDisabled(unsigned RuleID) const {\n"
+     << "RuleConfig::isRuleDisabled(unsigned RuleID) const {\n"
      << "  return DisabledRules.test(RuleID);\n"
      << "}\n";
   OS << "#endif // ifdef " << Name.upper() << "_GENCOMBINERHELPER_H\n\n";
 
   OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_CPP\n"
      << "\n"
      << "cl::list<std::string> " << Name << "Option(\n"
      << "    \"" << Name.lower() << "-disable-rule\",\n"
      << "    cl::desc(\"Disable one or more combiner rules temporarily in "
      << "the " << Name << " pass\"),\n"
      << "    cl::CommaSeparated,\n"
      << "    cl::Hidden,\n"
      << "    cl::cat(GICombinerOptionCategory));\n"
      << "\n"
-     << "bool " << getClassName() << "::parseCommandLineOption() {\n"
+     << "bool " << getClassName() << "RuleConfig::parseCommandLineOption() {\n"
      << "  for (const auto &Identifier : " << Name << "Option)\n"
      << "    if (!setRuleDisabled(Identifier))\n"
      << "      return false;\n"
      << "  return true;\n"
      << "}\n\n";
 
   OS << "bool " << getClassName() << "::tryCombineAll(\n"
      << "    GISelChangeObserver &Observer,\n"
      << "    MachineInstr &MI,\n"
      << "    MachineIRBuilder &B";
   emitAdditionalHelperMethodArguments(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";
+     << "  (void)MBB; (void)MF; (void)MRI; (void)RuleConfig;\n\n";
 
   OS << "  // Match data\n";
   for (const auto &Rule : Rules)
     for (const auto &I : Rule->matchdata_decls())
       OS << "  " << I.getType() << " " << I.getVariableName() << ";\n";
   OS << "\n";
 
   OS << "  int Partition = -1;\n";