[RFC][GlobalISel] Use Builders in MatchTable (#65955)

The MatchTableExecutor did not use the MachineIRBuilder but instead created instructions ad-hoc. Making it use a Builder has the benefit that any observer added by a combine is now notified when instructions are created by MIR patterns. Another benefit is that it allows me to improve how constants are created in apply MIR patterns. `MachineIRBuilder::buildConstant` automatically handles splats for us, this means that we may change `addCImm` to use that and handle vector cases automatically.
2023-10-16 07:41:18 +02:00
parent 94d0a3c4a8
commit 96e473a6be
6 changed files with 45 additions and 33 deletions
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
@@ -40,6 +40,7 @@ class APInt;
 class APFloat;
 class GISelKnownBits;
 class MachineInstr;
+class MachineIRBuilder;
 class MachineInstrBuilder;
 class MachineFunction;
 class MachineOperand;
@@ -555,15 +556,15 @@ protected:
  /// and false otherwise.
  template <class TgtExecutor, class PredicateBitset, class ComplexMatcherMemFn,
            class CustomRendererFn>
-  bool executeMatchTable(
-      TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
-      const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn, CustomRendererFn>
-          &ISelInfo,
-      const int64_t *MatchTable, const TargetInstrInfo &TII,
-      MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
-      const RegisterBankInfo &RBI, const PredicateBitset &AvailableFeatures,
-      CodeGenCoverage *CoverageInfo,
-      GISelChangeObserver *Observer = nullptr) const;
+  bool executeMatchTable(TgtExecutor &Exec, MatcherState &State,
+                         const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn,
+                                          CustomRendererFn> &ExecInfo,
+                         MachineIRBuilder &Builder, const int64_t *MatchTable,
+                         const TargetInstrInfo &TII, MachineRegisterInfo &MRI,
+                         const TargetRegisterInfo &TRI,
+                         const RegisterBankInfo &RBI,
+                         const PredicateBitset &AvailableFeatures,
+                         CodeGenCoverage *CoverageInfo) const;

  virtual const int64_t *getMatchTable() const {
    llvm_unreachable("Should have been overridden by tablegen if used");
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
 #include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
+#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOperand.h"
@@ -42,17 +43,20 @@ namespace llvm {
 template <class TgtExecutor, class PredicateBitset, class ComplexMatcherMemFn,
          class CustomRendererFn>
 bool GIMatchTableExecutor::executeMatchTable(
-    TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
+    TgtExecutor &Exec, MatcherState &State,
    const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn, CustomRendererFn>
        &ExecInfo,
-    const int64_t *MatchTable, const TargetInstrInfo &TII,
-    MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
-    const RegisterBankInfo &RBI, const PredicateBitset &AvailableFeatures,
-    CodeGenCoverage *CoverageInfo, GISelChangeObserver *Observer) const {
+    MachineIRBuilder &Builder, const int64_t *MatchTable,
+    const TargetInstrInfo &TII, MachineRegisterInfo &MRI,
+    const TargetRegisterInfo &TRI, const RegisterBankInfo &RBI,
+    const PredicateBitset &AvailableFeatures,
+    CodeGenCoverage *CoverageInfo) const {

  uint64_t CurrentIdx = 0;
  SmallVector<uint64_t, 4> OnFailResumeAt;
+  NewMIVector OutMIs;

+  GISelChangeObserver *Observer = Builder.getObserver();
  // Bypass the flag check on the instruction, and only look at the MCInstrDesc.
  bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();

@@ -71,14 +75,18 @@ bool GIMatchTableExecutor::executeMatchTable(
    return RejectAndResume;
  };

-  auto propagateFlags = [=](NewMIVector &OutMIs) {
+  const auto propagateFlags = [&]() {
    for (auto MIB : OutMIs) {
      // Set the NoFPExcept flag when no original matched instruction could
      // raise an FP exception, but the new instruction potentially might.
      uint16_t MIBFlags = Flags;
      if (NoFPException && MIB->mayRaiseFPException())
        MIBFlags |= MachineInstr::NoFPExcept;
+      if (Observer)
+        Observer->changingInstr(*MIB);
      MIB.setMIFlags(MIBFlags);
+      if (Observer)
+        Observer->changedInstr(*MIB);
    }

    return true;
@@ -898,9 +906,13 @@ bool GIMatchTableExecutor::executeMatchTable(
      if (NewInsnID >= OutMIs.size())
        OutMIs.resize(NewInsnID + 1);

-      OutMIs[NewInsnID] = MachineInstrBuilder(*State.MIs[OldInsnID]->getMF(),
-                                              State.MIs[OldInsnID]);
+      MachineInstr *OldMI = State.MIs[OldInsnID];
+      if (Observer)
+        Observer->changingInstr(*OldMI);
+      OutMIs[NewInsnID] = MachineInstrBuilder(*OldMI->getMF(), OldMI);
      OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
+      if (Observer)
+        Observer->changedInstr(*OldMI);
      DEBUG_WITH_TYPE(TgtExecutor::getName(),
                      dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
                             << NewInsnID << "], MIs[" << OldInsnID << "], "
@@ -914,8 +926,7 @@ bool GIMatchTableExecutor::executeMatchTable(
      if (NewInsnID >= OutMIs.size())
        OutMIs.resize(NewInsnID + 1);

-      OutMIs[NewInsnID] = BuildMI(*State.MIs[0]->getParent(), State.MIs[0],
-                                  MIMetadata(*State.MIs[0]), TII.get(Opcode));
+      OutMIs[NewInsnID] = Builder.buildInstr(Opcode);
      DEBUG_WITH_TYPE(TgtExecutor::getName(),
                      dbgs() << CurrentIdx << ": GIR_BuildMI(OutMIs["
                             << NewInsnID << "], " << Opcode << ")\n");
@@ -1239,6 +1250,10 @@ bool GIMatchTableExecutor::executeMatchTable(
      DEBUG_WITH_TYPE(TgtExecutor::getName(),
                      dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs["
                             << InsnID << "])\n");
+      // If we're erasing the insertion point, ensure we don't leave a dangling
+      // pointer in the builder.
+      if (Builder.getInsertPt() == MI)
+        Builder.setInsertPt(*MI->getParent(), ++MI->getIterator());
      if (Observer)
        Observer->erasingInstr(*MI);
      MI->eraseFromParent();
@@ -1309,11 +1324,7 @@ bool GIMatchTableExecutor::executeMatchTable(
    case GIR_Done:
      DEBUG_WITH_TYPE(TgtExecutor::getName(),
                      dbgs() << CurrentIdx << ": GIR_Done\n");
-      if (Observer) {
-        for (MachineInstr *MI : OutMIs)
-          Observer->createdInstr(*MI);
-      }
-      propagateFlags(OutMIs);
+      propagateFlags();
      return true;
    default:
      llvm_unreachable("Unexpected command");
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td
@@ -93,12 +93,12 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK:      bool GenMyCombiner::tryCombineAll(MachineInstr &I) const {
 // CHECK-NEXT:   const TargetSubtargetInfo &ST = MF.getSubtarget();
 // CHECK-NEXT:   const PredicateBitset AvailableFeatures = getAvailableFeatures();
-// CHECK-NEXT:   NewMIVector OutMIs;
+// CHECK-NEXT:   B.setInstrAndDebugLoc(I);
 // CHECK-NEXT:   State.MIs.clear();
 // CHECK-NEXT:   State.MIs.push_back(&I);
 // CHECK-NEXT:   MatchInfos = MatchInfosTy();
 // CHECK-EMPTY:
-// CHECK-NEXT:   if (executeMatchTable(*this, OutMIs, State, ExecInfo, getMatchTable(), *ST.getInstrInfo(), MRI, *MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures, /*CoverageInfo*/ nullptr, &Observer))
+// CHECK-NEXT:   if (executeMatchTable(*this, State, ExecInfo, B, getMatchTable(), *ST.getInstrInfo(), MRI, *MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures, /*CoverageInfo*/ nullptr))
 // CHECK-NEXT:     return true;
 // CHECK-NEXT:   }
 // CHECK-EMPTY:
--- a/llvm/test/TableGen/GlobalISelEmitter.td
+++ b/llvm/test/TableGen/GlobalISelEmitter.td
@@ -216,11 +216,11 @@ def HasC : Predicate<"Subtarget->hasC()"> { let RecomputePerFunction = 1; }

 // CHECK: bool MyTargetInstructionSelector::selectImpl(MachineInstr &I, CodeGenCoverage &CoverageInfo) const {
 // CHECK-NEXT: const PredicateBitset AvailableFeatures = getAvailableFeatures();
-// CHECK-NEXT: NewMIVector OutMIs;
+// CHECK-NEXT: MachineIRBuilder B(I);
 // CHECK-NEXT: State.MIs.clear();
 // CHECK-NEXT: State.MIs.push_back(&I);

-// CHECK:      if (executeMatchTable(*this, OutMIs, State, ExecInfo, getMatchTable(), TII, MF->getRegInfo(), TRI, RBI, AvailableFeatures, &CoverageInfo)) {
+// CHECK:      if (executeMatchTable(*this, State, ExecInfo, B, getMatchTable(), TII, MF->getRegInfo(), TRI, RBI, AvailableFeatures, &CoverageInfo)) {
 // CHECK-NEXT:   return true;
 // CHECK-NEXT: }

--- a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp
@@ -3465,15 +3465,15 @@ void GICombinerEmitter::emitAdditionalImpl(raw_ostream &OS) {
     << "  const TargetSubtargetInfo &ST = MF.getSubtarget();\n"
     << "  const PredicateBitset AvailableFeatures = "
        "getAvailableFeatures();\n"
-     << "  NewMIVector OutMIs;\n"
+     << "  B.setInstrAndDebugLoc(I);\n"
     << "  State.MIs.clear();\n"
     << "  State.MIs.push_back(&I);\n"
     << "  " << MatchDataInfo::StructName << " = "
     << MatchDataInfo::StructTypeName << "();\n\n"
-     << "  if (executeMatchTable(*this, OutMIs, State, ExecInfo"
+     << "  if (executeMatchTable(*this, State, ExecInfo, B"
     << ", getMatchTable(), *ST.getInstrInfo(), MRI, "
        "*MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures"
-     << ", /*CoverageInfo*/ nullptr, &Observer)) {\n"
+     << ", /*CoverageInfo*/ nullptr)) {\n"
     << "    return true;\n"
     << "  }\n\n"
     << "  return false;\n"
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -2267,10 +2267,10 @@ void GlobalISelEmitter::emitAdditionalImpl(raw_ostream &OS) {
        "&CoverageInfo) const {\n"
     << "  const PredicateBitset AvailableFeatures = "
        "getAvailableFeatures();\n"
-     << "  NewMIVector OutMIs;\n"
+     << "  MachineIRBuilder B(I);\n"
     << "  State.MIs.clear();\n"
     << "  State.MIs.push_back(&I);\n\n"
-     << "  if (executeMatchTable(*this, OutMIs, State, ExecInfo"
+     << "  if (executeMatchTable(*this, State, ExecInfo, B"
     << ", getMatchTable(), TII, MF->getRegInfo(), TRI, RBI, AvailableFeatures"
     << ", &CoverageInfo)) {\n"
     << "    return true;\n"