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

Pierre-vh · web-flow · commit 96e473a6be2e · 2023-10-16T07:41:18.000+02:00
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.
diff --git 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 @@ class GIMatchTableExecutor {
   /// 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");
diff --git 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");
diff --git 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:
diff --git 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: }
 
diff --git 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"
diff --git 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"
