Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GlobalISel] Improve Handling of Immediates in Apply MIR Patterns #66071

Closed
wants to merge 3 commits into from

Conversation

Pierre-vh
Copy link
Contributor

NOTE: This PR contains multiple commits as there is currently no way to do stacked pull requests on GitHub.
NOTE: The first commit is reviewed in #65955

This patch series aims to improve how immediates are materialized in the 'apply' patterns. It sidetracked a bit into also adding a GITypeOf type, which is needed to allow creating constants in apply that have the same type as the matched constant, but a different value.

Components:

  • ✅ Use MachineIRBuilder in the MatchTable
  • ✅ Use MachineIRBuilder::buildConstant to create constants in apply patterns.
  • ✅ Add GITypeOf<"$x"> special type to allow creating temp reg/immediates in apply pattern that have the same type as a matched pattern
  • 🕐 Add some basic type inference so GITypeOf is not needed in the common case

The MatchTableExecutor did not use the MachineIRBuilder nor Observers to build instructions.
This meant that it only had a very superficial view of what's going on when rules are being applied. Custom code could create insts that the executor didn't know about.

Using a builder & an observer allows it to collect any instruction that's created as long as the same builder is used by the supporting C++ code. For instance, flags are propagated more thoroughly now.

Another benefit that may come later is that I'm trying 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.
Use ``MachineIRBuilder::buildConstant`` to emit typed immediates in 'apply' MIR patterns.
This allows us to seamlessly handle vector cases, wherre a ``G_BUILD_VECTOR`` is needed to create a splat.

Depends on llvm#65955
Allows creating a register/immediate that uses the same type as a matched operand.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 12, 2023

@llvm/pr-subscribers-backend-aarch64

Changes

NOTE: This PR contains multiple commits as there is currently no way to do stacked pull requests on GitHub.
NOTE: The first commit is reviewed in #65955

This patch series aims to improve how immediates are materialized in the 'apply' patterns. It sidetracked a bit into also adding a GITypeOf type, which is needed to allow creating constants in apply that have the same type as the matched constant, but a different value.

Components:

  • ✅ Use MachineIRBuilder in the MatchTable
  • ✅ Use MachineIRBuilder::buildConstant to create constants in apply patterns.
  • ✅ Add GITypeOf<"$x"> special type to allow creating temp reg/immediates in apply pattern that have the same type as a matched pattern
  • 🕐 Add some basic type inference so GITypeOf is not needed in the common case
    --

Patch is 137.54 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66071.diff

26 Files Affected:

  • (modified) llvm/docs/GlobalISel/MIRPatterns.rst (+2-2)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h (+43-17)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h (+79-46)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+10)
  • (modified) llvm/include/llvm/Target/GlobalISel/Combine.td (+18)
  • (modified) llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp (+4)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-sdiv.mir (+14-14)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-ashr-shl-to-sext-inreg.mir (+2-2)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-add-mul-pre-legalize.mir (+48-48)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-unmerge-values.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fold-binop-into-select.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fsub-fneg.mir (+14-14)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-imms.td (+5-7)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-patfrag-root.td (+9-15)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td (+26-42)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td (+49)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td (+6-8)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td (+27-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td (+24-1)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td (+72)
  • (modified) llvm/test/TableGen/GlobalISelEmitter.td (+3-3)
  • (modified) llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp (+239-53)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+4-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.cpp (+42-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.h (+100-8)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTableExecutorEmitter.cpp (+1-1)
diff --git a/llvm/docs/GlobalISel/MIRPatterns.rst b/llvm/docs/GlobalISel/MIRPatterns.rst
index 51d1850a1236039..fa70311f48572de 100644
--- a/llvm/docs/GlobalISel/MIRPatterns.rst
+++ b/llvm/docs/GlobalISel/MIRPatterns.rst
@@ -257,8 +257,8 @@ Common Pattern #3: Emitting a Constant Value
 When an immediate operand appears in an 'apply' pattern, the behavior
 depends on whether it's typed or not.
 
-* If the immediate is typed, a ``G_CONSTANT`` is implicitly emitted
-  (= a register operand is added to the instruction).
+* If the immediate is typed, ``MachineIRBuilder::buildConstant`` is used
+  to create a ``G_CONSTANT``. A ``G_BUILD_VECTOR`` will be used for vectors.
 * If the immediate is untyped, a simple immediate is added
   (``MachineInstrBuilder::addImm``).
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
index 2b0733cf9353e6c..1af12e1017c34bf 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/Bitset.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/LowLevelType.h"
 #include "llvm/CodeGen/MachineFunction.h"
@@ -40,6 +41,7 @@ class APInt;
 class APFloat;
 class GISelKnownBits;
 class MachineInstr;
+class MachineIRBuilder;
 class MachineInstrBuilder;
 class MachineFunction;
 class MachineOperand;
@@ -274,6 +276,12 @@ enum {
   /// - StoreIdx - Store location in RecordedOperands.
   GIM_RecordNamedOperand,
 
+  /// TODO DESC
+  /// - InsnID - Instruction ID
+  /// - OpIdx - Operand index
+  /// - TempTypeIdx - Temp Type Index, always negative.
+  GIM_RecordRegType,
+
   /// Fail the current try-block, or completely fail to match if there is no
   /// current try-block.
   GIM_Reject,
@@ -291,6 +299,11 @@ enum {
   /// - Opcode - The new opcode to use
   GIR_BuildMI,
 
+  /// Builds a constant and stores its result in a TempReg.
+  /// - TempRegID - Temp Register to define.
+  /// - Imm - The immediate to add
+  GIR_BuildConstant,
+
   /// Copy an operand to the specified instruction
   /// - NewInsnID - Instruction ID to modify
   /// - OldInsnID - Instruction ID to copy from
@@ -350,12 +363,6 @@ enum {
   /// - Imm - The immediate to add
   GIR_AddImm,
 
-  /// Add an CImm to the specified instruction
-  /// - InsnID - Instruction ID to modify
-  /// - Ty - Type of the constant immediate.
-  /// - Imm - The immediate to add
-  GIR_AddCImm,
-
   /// Render complex operands to the specified instruction
   /// - InsnID - Instruction ID to modify
   /// - RendererID - The renderer to call
@@ -501,10 +508,25 @@ class GIMatchTableExecutor {
   }
 
 protected:
+  /// Observer used by \ref executeMatchTable to record all instructions created
+  /// by the rule.
+  class GIMatchTableObserver : public GISelChangeObserver {
+  public:
+    virtual ~GIMatchTableObserver();
+
+    void erasingInstr(MachineInstr &MI) override { CreatedInsts.erase(&MI); }
+    void createdInstr(MachineInstr &MI) override { CreatedInsts.insert(&MI); }
+    void changingInstr(MachineInstr &MI) override {}
+    void changedInstr(MachineInstr &MI) override {}
+
+    // Keeps track of all instructions that have been created when applying a
+    // rule.
+    SmallDenseSet CreatedInsts;
+  };
+
   using ComplexRendererFns =
       std::optional, 4>>;
   using RecordedMIVector = SmallVector;
-  using NewMIVector = SmallVector;
 
   struct MatcherState {
     std::vector Renderers;
@@ -516,6 +538,10 @@ class GIMatchTableExecutor {
     /// list. Currently such predicates don't have more then 3 arguments.
     std::array RecordedOperands;
 
+    /// Types extracted from an instruction's operand.
+    /// Whenever a type index is negative, we look here instead.
+    SmallVector RecordedTypes;
+
     MatcherState(unsigned MaxRenderers);
   };
 
@@ -555,15 +581,15 @@ class GIMatchTableExecutor {
   /// and false otherwise.
   template 
-  bool executeMatchTable(
-      TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
-      const ExecInfoTy
-          &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 &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");
@@ -592,7 +618,7 @@ class GIMatchTableExecutor {
   }
 
   virtual void runCustomAction(unsigned, const MatcherState &State,
-                               NewMIVector &OutMIs) const {
+                               ArrayRef OutMIs) const {
     llvm_unreachable("Subclass does not implement runCustomAction!");
   }
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
index 883c1ca0fe350b0..fa8bddf795a9a7f 100644
--- 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,33 @@ namespace llvm {
 template 
 bool GIMatchTableExecutor::executeMatchTable(
-    TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
+    TgtExecutor &Exec, MatcherState &State,
     const ExecInfoTy
         &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 {
+
+  // Setup observer
+  GIMatchTableObserver MTObserver;
+  GISelObserverWrapper Observer(&MTObserver);
+  if (auto *CurObs = Builder.getChangeObserver())
+    Observer.addObserver(CurObs);
+
+  // TODO: Set MF delegate?
+
+  // Setup builder.
+  auto RestoreOldObserver = Builder.setTemporaryChangeObserver(Observer);
 
   uint64_t CurrentIdx = 0;
   SmallVector OnFailResumeAt;
 
+  // We also record MachineInstrs manually in this vector so opcodes can address
+  // them.
+  SmallVector OutMIs;
+
   // Bypass the flag check on the instruction, and only look at the MCInstrDesc.
   bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();
 
@@ -71,19 +88,29 @@ bool GIMatchTableExecutor::executeMatchTable(
     return RejectAndResume;
   };
 
-  auto propagateFlags = [=](NewMIVector &OutMIs) {
-    for (auto MIB : OutMIs) {
+  auto propagateFlags = [&]() {
+    for (auto *MI : MTObserver.CreatedInsts) {
       // 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())
+      if (NoFPException && MI->mayRaiseFPException())
         MIBFlags |= MachineInstr::NoFPExcept;
-      MIB.setMIFlags(MIBFlags);
+      Observer.changingInstr(*MI);
+      MI->setFlags(MIBFlags);
+      Observer.changedInstr(*MI);
     }
 
     return true;
   };
 
+  // If the index is >= 0, it's an index in the type objects generated by TableGen.
+  // If the index is <0, it's an index in the recorded types object.
+  auto getTypeFromIdx = [&](int64_t Idx) -> const LLT& {
+    if(Idx >= 0)
+      return ExecInfo.TypeObjects[Idx];
+    return State.RecordedTypes[1 - Idx];
+  };
+
   while (true) {
     assert(CurrentIdx != ~0u && "Invalid MatchTable index");
     int64_t MatcherOpcode = MatchTable[CurrentIdx++];
@@ -620,7 +647,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
       MachineOperand &MO = State.MIs[InsnID]->getOperand(OpIdx);
       if (!MO.isReg() ||
-          MRI.getType(MO.getReg()) != ExecInfo.TypeObjects[TypeID]) {
+          MRI.getType(MO.getReg()) != getTypeFromIdx(TypeID)) {
         if (handleReject() == RejectAndGiveUp)
           return false;
       }
@@ -671,6 +698,25 @@ bool GIMatchTableExecutor::executeMatchTable(
       State.RecordedOperands[StoreIdx] = &State.MIs[InsnID]->getOperand(OpIdx);
       break;
     }
+    case GIM_RecordRegType: {
+      int64_t InsnID = MatchTable[CurrentIdx++];
+      int64_t OpIdx = MatchTable[CurrentIdx++];
+      int64_t TypeIdx = MatchTable[CurrentIdx++];
+
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIM_RecordRegType(MIs["
+                             << InsnID << "]->getOperand(" << OpIdx
+                             << "), TypeIdx=" << TypeIdx << ")\n");
+      assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+      assert(TypeIdx <= 0 && "Temp types always have negative indexes!");
+      // Indexes start at -1.
+      TypeIdx = 1 - TypeIdx;
+      const auto& Op = State.MIs[InsnID]->getOperand(OpIdx);
+      if(State.RecordedTypes.size() <= (uint64_t)TypeIdx)
+        State.RecordedTypes.resize(TypeIdx + 1, LLT());
+      State.RecordedTypes[TypeIdx] = MRI.getType(Op.getReg());
+      break;
+    }
     case GIM_CheckRegBankForClass: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t OpIdx = MatchTable[CurrentIdx++];
@@ -901,6 +947,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       OutMIs[NewInsnID] = MachineInstrBuilder(*State.MIs[OldInsnID]->getMF(),
                                               State.MIs[OldInsnID]);
       OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
+      MTObserver.CreatedInsts.insert(OutMIs[NewInsnID]);
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
                              << NewInsnID << "], MIs[" << OldInsnID << "], "
@@ -914,14 +961,23 @@ 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");
       break;
     }
 
+    case GIR_BuildConstant: {
+      int64_t TempRegID = MatchTable[CurrentIdx++];
+      int64_t Imm = MatchTable[CurrentIdx++];
+      Builder.buildConstant(State.TempRegisters[TempRegID], Imm);
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIR_BuildConstant(TempReg["
+                             << TempRegID << "], Imm=" << Imm << ")\n");
+      break;
+    }
+
     case GIR_Copy: {
       int64_t NewInsnID = MatchTable[CurrentIdx++];
       int64_t OldInsnID = MatchTable[CurrentIdx++];
@@ -1047,24 +1103,6 @@ bool GIMatchTableExecutor::executeMatchTable(
                              << "], " << Imm << ")\n");
       break;
     }
-
-    case GIR_AddCImm: {
-      int64_t InsnID = MatchTable[CurrentIdx++];
-      int64_t TypeID = MatchTable[CurrentIdx++];
-      int64_t Imm = MatchTable[CurrentIdx++];
-      assert(OutMIs[InsnID] && "Attempted to add to undefined instruction");
-
-      unsigned Width = ExecInfo.TypeObjects[TypeID].getScalarSizeInBits();
-      LLVMContext &Ctx = MF->getFunction().getContext();
-      OutMIs[InsnID].addCImm(
-          ConstantInt::get(IntegerType::get(Ctx, Width), Imm, /*signed*/ true));
-      DEBUG_WITH_TYPE(TgtExecutor::getName(),
-                      dbgs() << CurrentIdx << ": GIR_AddCImm(OutMIs[" << InsnID
-                             << "], TypeID=" << TypeID << ", Imm=" << Imm
-                             << ")\n");
-      break;
-    }
-
     case GIR_ComplexRenderer: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t RendererID = MatchTable[CurrentIdx++];
@@ -1239,8 +1277,11 @@ bool GIMatchTableExecutor::executeMatchTable(
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs["
                              << InsnID << "])\n");
-      if (Observer)
-        Observer->erasingInstr(*MI);
+      // 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());
+      Observer.erasingInstr(*MI);
       MI->eraseFromParent();
       break;
     }
@@ -1250,7 +1291,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       int64_t TypeID = MatchTable[CurrentIdx++];
 
       State.TempRegisters[TempRegID] =
-          MRI.createGenericVirtualRegister(ExecInfo.TypeObjects[TypeID]);
+          MRI.createGenericVirtualRegister(getTypeFromIdx(TypeID));
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": TempRegs[" << TempRegID
                              << "] = GIR_MakeTempReg(" << TypeID << ")\n");
@@ -1269,11 +1310,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.MIs[NewInsnID]->getOperand(NewOpIdx).getReg();
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_ReplaceRegWithTempReg: {
@@ -1288,11 +1327,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.TempRegisters[TempRegID];
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_Coverage: {
@@ -1309,11 +1346,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/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index e7db9547f03b694..99b821406b0f893 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -20,6 +20,7 @@
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/SaveAndRestore.h"
 
 namespace llvm {
 
@@ -364,6 +365,15 @@ class MachineIRBuilder {
     State.Observer = &Observer;
   }
 
+  GISelChangeObserver *getChangeObserver() const { return State.Observer; }
+
+  // Replaces the change observer with \p Observer and returns an object that
+  // restores the old Observer on destruction.
+  SaveAndRestore
+  setTemporaryChangeObserver(GISelChangeObserver &Observer) {
+    return SaveAndRestore(State.Observer, &Observer);
+  }
+
   void stopObservingChanges() { State.Observer = nullptr; }
 
   bool isObservingChanges() const { return State.Observer != nullptr; }
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 7e0691e1ee95048..4a22dd61c6e9b04 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -110,6 +110,24 @@ class GICombinePatFrag alts> {
   list Alternatives = alts;
 }
 
+//===----------------------------------------------------------------------===//
+// Pattern Special Types
+//===----------------------------------------------------------------------===//
+
+class GISpecialType;
+
+// In an apply pattern, GITypeOf can be used to set the type of a ne...

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 12, 2023

@llvm/pr-subscribers-backend-amdgpu

Changes

NOTE: This PR contains multiple commits as there is currently no way to do stacked pull requests on GitHub.
NOTE: The first commit is reviewed in #65955

This patch series aims to improve how immediates are materialized in the 'apply' patterns. It sidetracked a bit into also adding a GITypeOf type, which is needed to allow creating constants in apply that have the same type as the matched constant, but a different value.

Components:

  • ✅ Use MachineIRBuilder in the MatchTable
  • ✅ Use MachineIRBuilder::buildConstant to create constants in apply patterns.
  • ✅ Add GITypeOf<"$x"> special type to allow creating temp reg/immediates in apply pattern that have the same type as a matched pattern
  • 🕐 Add some basic type inference so GITypeOf is not needed in the common case
    --

Patch is 137.54 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66071.diff

26 Files Affected:

  • (modified) llvm/docs/GlobalISel/MIRPatterns.rst (+2-2)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h (+43-17)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h (+79-46)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+10)
  • (modified) llvm/include/llvm/Target/GlobalISel/Combine.td (+18)
  • (modified) llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp (+4)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-sdiv.mir (+14-14)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-ashr-shl-to-sext-inreg.mir (+2-2)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-add-mul-pre-legalize.mir (+48-48)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-unmerge-values.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fold-binop-into-select.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fsub-fneg.mir (+14-14)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-imms.td (+5-7)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-patfrag-root.td (+9-15)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td (+26-42)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td (+49)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td (+6-8)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td (+27-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td (+24-1)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td (+72)
  • (modified) llvm/test/TableGen/GlobalISelEmitter.td (+3-3)
  • (modified) llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp (+239-53)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+4-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.cpp (+42-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.h (+100-8)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTableExecutorEmitter.cpp (+1-1)
diff --git a/llvm/docs/GlobalISel/MIRPatterns.rst b/llvm/docs/GlobalISel/MIRPatterns.rst
index 51d1850a1236039..fa70311f48572de 100644
--- a/llvm/docs/GlobalISel/MIRPatterns.rst
+++ b/llvm/docs/GlobalISel/MIRPatterns.rst
@@ -257,8 +257,8 @@ Common Pattern #3: Emitting a Constant Value
 When an immediate operand appears in an 'apply' pattern, the behavior
 depends on whether it's typed or not.
 
-* If the immediate is typed, a ``G_CONSTANT`` is implicitly emitted
-  (= a register operand is added to the instruction).
+* If the immediate is typed, ``MachineIRBuilder::buildConstant`` is used
+  to create a ``G_CONSTANT``. A ``G_BUILD_VECTOR`` will be used for vectors.
 * If the immediate is untyped, a simple immediate is added
   (``MachineInstrBuilder::addImm``).
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
index 2b0733cf9353e6c..1af12e1017c34bf 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/Bitset.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/LowLevelType.h"
 #include "llvm/CodeGen/MachineFunction.h"
@@ -40,6 +41,7 @@ class APInt;
 class APFloat;
 class GISelKnownBits;
 class MachineInstr;
+class MachineIRBuilder;
 class MachineInstrBuilder;
 class MachineFunction;
 class MachineOperand;
@@ -274,6 +276,12 @@ enum {
   /// - StoreIdx - Store location in RecordedOperands.
   GIM_RecordNamedOperand,
 
+  /// TODO DESC
+  /// - InsnID - Instruction ID
+  /// - OpIdx - Operand index
+  /// - TempTypeIdx - Temp Type Index, always negative.
+  GIM_RecordRegType,
+
   /// Fail the current try-block, or completely fail to match if there is no
   /// current try-block.
   GIM_Reject,
@@ -291,6 +299,11 @@ enum {
   /// - Opcode - The new opcode to use
   GIR_BuildMI,
 
+  /// Builds a constant and stores its result in a TempReg.
+  /// - TempRegID - Temp Register to define.
+  /// - Imm - The immediate to add
+  GIR_BuildConstant,
+
   /// Copy an operand to the specified instruction
   /// - NewInsnID - Instruction ID to modify
   /// - OldInsnID - Instruction ID to copy from
@@ -350,12 +363,6 @@ enum {
   /// - Imm - The immediate to add
   GIR_AddImm,
 
-  /// Add an CImm to the specified instruction
-  /// - InsnID - Instruction ID to modify
-  /// - Ty - Type of the constant immediate.
-  /// - Imm - The immediate to add
-  GIR_AddCImm,
-
   /// Render complex operands to the specified instruction
   /// - InsnID - Instruction ID to modify
   /// - RendererID - The renderer to call
@@ -501,10 +508,25 @@ class GIMatchTableExecutor {
   }
 
 protected:
+  /// Observer used by \ref executeMatchTable to record all instructions created
+  /// by the rule.
+  class GIMatchTableObserver : public GISelChangeObserver {
+  public:
+    virtual ~GIMatchTableObserver();
+
+    void erasingInstr(MachineInstr &MI) override { CreatedInsts.erase(&MI); }
+    void createdInstr(MachineInstr &MI) override { CreatedInsts.insert(&MI); }
+    void changingInstr(MachineInstr &MI) override {}
+    void changedInstr(MachineInstr &MI) override {}
+
+    // Keeps track of all instructions that have been created when applying a
+    // rule.
+    SmallDenseSet CreatedInsts;
+  };
+
   using ComplexRendererFns =
       std::optional, 4>>;
   using RecordedMIVector = SmallVector;
-  using NewMIVector = SmallVector;
 
   struct MatcherState {
     std::vector Renderers;
@@ -516,6 +538,10 @@ class GIMatchTableExecutor {
     /// list. Currently such predicates don't have more then 3 arguments.
     std::array RecordedOperands;
 
+    /// Types extracted from an instruction's operand.
+    /// Whenever a type index is negative, we look here instead.
+    SmallVector RecordedTypes;
+
     MatcherState(unsigned MaxRenderers);
   };
 
@@ -555,15 +581,15 @@ class GIMatchTableExecutor {
   /// and false otherwise.
   template 
-  bool executeMatchTable(
-      TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
-      const ExecInfoTy
-          &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 &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");
@@ -592,7 +618,7 @@ class GIMatchTableExecutor {
   }
 
   virtual void runCustomAction(unsigned, const MatcherState &State,
-                               NewMIVector &OutMIs) const {
+                               ArrayRef OutMIs) const {
     llvm_unreachable("Subclass does not implement runCustomAction!");
   }
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
index 883c1ca0fe350b0..fa8bddf795a9a7f 100644
--- 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,33 @@ namespace llvm {
 template 
 bool GIMatchTableExecutor::executeMatchTable(
-    TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
+    TgtExecutor &Exec, MatcherState &State,
     const ExecInfoTy
         &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 {
+
+  // Setup observer
+  GIMatchTableObserver MTObserver;
+  GISelObserverWrapper Observer(&MTObserver);
+  if (auto *CurObs = Builder.getChangeObserver())
+    Observer.addObserver(CurObs);
+
+  // TODO: Set MF delegate?
+
+  // Setup builder.
+  auto RestoreOldObserver = Builder.setTemporaryChangeObserver(Observer);
 
   uint64_t CurrentIdx = 0;
   SmallVector OnFailResumeAt;
 
+  // We also record MachineInstrs manually in this vector so opcodes can address
+  // them.
+  SmallVector OutMIs;
+
   // Bypass the flag check on the instruction, and only look at the MCInstrDesc.
   bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();
 
@@ -71,19 +88,29 @@ bool GIMatchTableExecutor::executeMatchTable(
     return RejectAndResume;
   };
 
-  auto propagateFlags = [=](NewMIVector &OutMIs) {
-    for (auto MIB : OutMIs) {
+  auto propagateFlags = [&]() {
+    for (auto *MI : MTObserver.CreatedInsts) {
       // 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())
+      if (NoFPException && MI->mayRaiseFPException())
         MIBFlags |= MachineInstr::NoFPExcept;
-      MIB.setMIFlags(MIBFlags);
+      Observer.changingInstr(*MI);
+      MI->setFlags(MIBFlags);
+      Observer.changedInstr(*MI);
     }
 
     return true;
   };
 
+  // If the index is >= 0, it's an index in the type objects generated by TableGen.
+  // If the index is <0, it's an index in the recorded types object.
+  auto getTypeFromIdx = [&](int64_t Idx) -> const LLT& {
+    if(Idx >= 0)
+      return ExecInfo.TypeObjects[Idx];
+    return State.RecordedTypes[1 - Idx];
+  };
+
   while (true) {
     assert(CurrentIdx != ~0u && "Invalid MatchTable index");
     int64_t MatcherOpcode = MatchTable[CurrentIdx++];
@@ -620,7 +647,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
       MachineOperand &MO = State.MIs[InsnID]->getOperand(OpIdx);
       if (!MO.isReg() ||
-          MRI.getType(MO.getReg()) != ExecInfo.TypeObjects[TypeID]) {
+          MRI.getType(MO.getReg()) != getTypeFromIdx(TypeID)) {
         if (handleReject() == RejectAndGiveUp)
           return false;
       }
@@ -671,6 +698,25 @@ bool GIMatchTableExecutor::executeMatchTable(
       State.RecordedOperands[StoreIdx] = &State.MIs[InsnID]->getOperand(OpIdx);
       break;
     }
+    case GIM_RecordRegType: {
+      int64_t InsnID = MatchTable[CurrentIdx++];
+      int64_t OpIdx = MatchTable[CurrentIdx++];
+      int64_t TypeIdx = MatchTable[CurrentIdx++];
+
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIM_RecordRegType(MIs["
+                             << InsnID << "]->getOperand(" << OpIdx
+                             << "), TypeIdx=" << TypeIdx << ")\n");
+      assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+      assert(TypeIdx <= 0 && "Temp types always have negative indexes!");
+      // Indexes start at -1.
+      TypeIdx = 1 - TypeIdx;
+      const auto& Op = State.MIs[InsnID]->getOperand(OpIdx);
+      if(State.RecordedTypes.size() <= (uint64_t)TypeIdx)
+        State.RecordedTypes.resize(TypeIdx + 1, LLT());
+      State.RecordedTypes[TypeIdx] = MRI.getType(Op.getReg());
+      break;
+    }
     case GIM_CheckRegBankForClass: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t OpIdx = MatchTable[CurrentIdx++];
@@ -901,6 +947,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       OutMIs[NewInsnID] = MachineInstrBuilder(*State.MIs[OldInsnID]->getMF(),
                                               State.MIs[OldInsnID]);
       OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
+      MTObserver.CreatedInsts.insert(OutMIs[NewInsnID]);
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
                              << NewInsnID << "], MIs[" << OldInsnID << "], "
@@ -914,14 +961,23 @@ 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");
       break;
     }
 
+    case GIR_BuildConstant: {
+      int64_t TempRegID = MatchTable[CurrentIdx++];
+      int64_t Imm = MatchTable[CurrentIdx++];
+      Builder.buildConstant(State.TempRegisters[TempRegID], Imm);
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIR_BuildConstant(TempReg["
+                             << TempRegID << "], Imm=" << Imm << ")\n");
+      break;
+    }
+
     case GIR_Copy: {
       int64_t NewInsnID = MatchTable[CurrentIdx++];
       int64_t OldInsnID = MatchTable[CurrentIdx++];
@@ -1047,24 +1103,6 @@ bool GIMatchTableExecutor::executeMatchTable(
                              << "], " << Imm << ")\n");
       break;
     }
-
-    case GIR_AddCImm: {
-      int64_t InsnID = MatchTable[CurrentIdx++];
-      int64_t TypeID = MatchTable[CurrentIdx++];
-      int64_t Imm = MatchTable[CurrentIdx++];
-      assert(OutMIs[InsnID] && "Attempted to add to undefined instruction");
-
-      unsigned Width = ExecInfo.TypeObjects[TypeID].getScalarSizeInBits();
-      LLVMContext &Ctx = MF->getFunction().getContext();
-      OutMIs[InsnID].addCImm(
-          ConstantInt::get(IntegerType::get(Ctx, Width), Imm, /*signed*/ true));
-      DEBUG_WITH_TYPE(TgtExecutor::getName(),
-                      dbgs() << CurrentIdx << ": GIR_AddCImm(OutMIs[" << InsnID
-                             << "], TypeID=" << TypeID << ", Imm=" << Imm
-                             << ")\n");
-      break;
-    }
-
     case GIR_ComplexRenderer: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t RendererID = MatchTable[CurrentIdx++];
@@ -1239,8 +1277,11 @@ bool GIMatchTableExecutor::executeMatchTable(
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs["
                              << InsnID << "])\n");
-      if (Observer)
-        Observer->erasingInstr(*MI);
+      // 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());
+      Observer.erasingInstr(*MI);
       MI->eraseFromParent();
       break;
     }
@@ -1250,7 +1291,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       int64_t TypeID = MatchTable[CurrentIdx++];
 
       State.TempRegisters[TempRegID] =
-          MRI.createGenericVirtualRegister(ExecInfo.TypeObjects[TypeID]);
+          MRI.createGenericVirtualRegister(getTypeFromIdx(TypeID));
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": TempRegs[" << TempRegID
                              << "] = GIR_MakeTempReg(" << TypeID << ")\n");
@@ -1269,11 +1310,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.MIs[NewInsnID]->getOperand(NewOpIdx).getReg();
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_ReplaceRegWithTempReg: {
@@ -1288,11 +1327,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.TempRegisters[TempRegID];
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_Coverage: {
@@ -1309,11 +1346,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/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index e7db9547f03b694..99b821406b0f893 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -20,6 +20,7 @@
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/SaveAndRestore.h"
 
 namespace llvm {
 
@@ -364,6 +365,15 @@ class MachineIRBuilder {
     State.Observer = &Observer;
   }
 
+  GISelChangeObserver *getChangeObserver() const { return State.Observer; }
+
+  // Replaces the change observer with \p Observer and returns an object that
+  // restores the old Observer on destruction.
+  SaveAndRestore
+  setTemporaryChangeObserver(GISelChangeObserver &Observer) {
+    return SaveAndRestore(State.Observer, &Observer);
+  }
+
   void stopObservingChanges() { State.Observer = nullptr; }
 
   bool isObservingChanges() const { return State.Observer != nullptr; }
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 7e0691e1ee95048..4a22dd61c6e9b04 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -110,6 +110,24 @@ class GICombinePatFrag alts> {
   list Alternatives = alts;
 }
 
+//===----------------------------------------------------------------------===//
+// Pattern Special Types
+//===----------------------------------------------------------------------===//
+
+class GISpecialType;
+
+// In an apply pattern, GITypeOf can be used to set the type of a ne...

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 12, 2023

@llvm/pr-subscribers-llvm-globalisel

Changes

NOTE: This PR contains multiple commits as there is currently no way to do stacked pull requests on GitHub.
NOTE: The first commit is reviewed in #65955

This patch series aims to improve how immediates are materialized in the 'apply' patterns. It sidetracked a bit into also adding a GITypeOf type, which is needed to allow creating constants in apply that have the same type as the matched constant, but a different value.

Components:

  • ✅ Use MachineIRBuilder in the MatchTable
  • ✅ Use MachineIRBuilder::buildConstant to create constants in apply patterns.
  • ✅ Add GITypeOf<"$x"> special type to allow creating temp reg/immediates in apply pattern that have the same type as a matched pattern
  • 🕐 Add some basic type inference so GITypeOf is not needed in the common case
    --

Patch is 137.54 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66071.diff

26 Files Affected:

  • (modified) llvm/docs/GlobalISel/MIRPatterns.rst (+2-2)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h (+43-17)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h (+79-46)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+10)
  • (modified) llvm/include/llvm/Target/GlobalISel/Combine.td (+18)
  • (modified) llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp (+4)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-sdiv.mir (+14-14)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-ashr-shl-to-sext-inreg.mir (+2-2)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-add-mul-pre-legalize.mir (+48-48)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fma-unmerge-values.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fold-binop-into-select.mir (+10-10)
  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fsub-fneg.mir (+14-14)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-imms.td (+5-7)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-patfrag-root.td (+9-15)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td (+26-42)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td (+49)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td (+6-8)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td (+27-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td (+24-1)
  • (added) llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td (+72)
  • (modified) llvm/test/TableGen/GlobalISelEmitter.td (+3-3)
  • (modified) llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp (+239-53)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+4-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.cpp (+42-3)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.h (+100-8)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTableExecutorEmitter.cpp (+1-1)
diff --git a/llvm/docs/GlobalISel/MIRPatterns.rst b/llvm/docs/GlobalISel/MIRPatterns.rst
index 51d1850a1236039..fa70311f48572de 100644
--- a/llvm/docs/GlobalISel/MIRPatterns.rst
+++ b/llvm/docs/GlobalISel/MIRPatterns.rst
@@ -257,8 +257,8 @@ Common Pattern #3: Emitting a Constant Value
 When an immediate operand appears in an 'apply' pattern, the behavior
 depends on whether it's typed or not.
 
-* If the immediate is typed, a ``G_CONSTANT`` is implicitly emitted
-  (= a register operand is added to the instruction).
+* If the immediate is typed, ``MachineIRBuilder::buildConstant`` is used
+  to create a ``G_CONSTANT``. A ``G_BUILD_VECTOR`` will be used for vectors.
 * If the immediate is untyped, a simple immediate is added
   (``MachineInstrBuilder::addImm``).
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
index 2b0733cf9353e6c..1af12e1017c34bf 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/Bitset.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/LowLevelType.h"
 #include "llvm/CodeGen/MachineFunction.h"
@@ -40,6 +41,7 @@ class APInt;
 class APFloat;
 class GISelKnownBits;
 class MachineInstr;
+class MachineIRBuilder;
 class MachineInstrBuilder;
 class MachineFunction;
 class MachineOperand;
@@ -274,6 +276,12 @@ enum {
   /// - StoreIdx - Store location in RecordedOperands.
   GIM_RecordNamedOperand,
 
+  /// TODO DESC
+  /// - InsnID - Instruction ID
+  /// - OpIdx - Operand index
+  /// - TempTypeIdx - Temp Type Index, always negative.
+  GIM_RecordRegType,
+
   /// Fail the current try-block, or completely fail to match if there is no
   /// current try-block.
   GIM_Reject,
@@ -291,6 +299,11 @@ enum {
   /// - Opcode - The new opcode to use
   GIR_BuildMI,
 
+  /// Builds a constant and stores its result in a TempReg.
+  /// - TempRegID - Temp Register to define.
+  /// - Imm - The immediate to add
+  GIR_BuildConstant,
+
   /// Copy an operand to the specified instruction
   /// - NewInsnID - Instruction ID to modify
   /// - OldInsnID - Instruction ID to copy from
@@ -350,12 +363,6 @@ enum {
   /// - Imm - The immediate to add
   GIR_AddImm,
 
-  /// Add an CImm to the specified instruction
-  /// - InsnID - Instruction ID to modify
-  /// - Ty - Type of the constant immediate.
-  /// - Imm - The immediate to add
-  GIR_AddCImm,
-
   /// Render complex operands to the specified instruction
   /// - InsnID - Instruction ID to modify
   /// - RendererID - The renderer to call
@@ -501,10 +508,25 @@ class GIMatchTableExecutor {
   }
 
 protected:
+  /// Observer used by \ref executeMatchTable to record all instructions created
+  /// by the rule.
+  class GIMatchTableObserver : public GISelChangeObserver {
+  public:
+    virtual ~GIMatchTableObserver();
+
+    void erasingInstr(MachineInstr &MI) override { CreatedInsts.erase(&MI); }
+    void createdInstr(MachineInstr &MI) override { CreatedInsts.insert(&MI); }
+    void changingInstr(MachineInstr &MI) override {}
+    void changedInstr(MachineInstr &MI) override {}
+
+    // Keeps track of all instructions that have been created when applying a
+    // rule.
+    SmallDenseSet CreatedInsts;
+  };
+
   using ComplexRendererFns =
       std::optional, 4>>;
   using RecordedMIVector = SmallVector;
-  using NewMIVector = SmallVector;
 
   struct MatcherState {
     std::vector Renderers;
@@ -516,6 +538,10 @@ class GIMatchTableExecutor {
     /// list. Currently such predicates don't have more then 3 arguments.
     std::array RecordedOperands;
 
+    /// Types extracted from an instruction's operand.
+    /// Whenever a type index is negative, we look here instead.
+    SmallVector RecordedTypes;
+
     MatcherState(unsigned MaxRenderers);
   };
 
@@ -555,15 +581,15 @@ class GIMatchTableExecutor {
   /// and false otherwise.
   template 
-  bool executeMatchTable(
-      TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
-      const ExecInfoTy
-          &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 &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");
@@ -592,7 +618,7 @@ class GIMatchTableExecutor {
   }
 
   virtual void runCustomAction(unsigned, const MatcherState &State,
-                               NewMIVector &OutMIs) const {
+                               ArrayRef OutMIs) const {
     llvm_unreachable("Subclass does not implement runCustomAction!");
   }
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
index 883c1ca0fe350b0..fa8bddf795a9a7f 100644
--- 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,33 @@ namespace llvm {
 template 
 bool GIMatchTableExecutor::executeMatchTable(
-    TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
+    TgtExecutor &Exec, MatcherState &State,
     const ExecInfoTy
         &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 {
+
+  // Setup observer
+  GIMatchTableObserver MTObserver;
+  GISelObserverWrapper Observer(&MTObserver);
+  if (auto *CurObs = Builder.getChangeObserver())
+    Observer.addObserver(CurObs);
+
+  // TODO: Set MF delegate?
+
+  // Setup builder.
+  auto RestoreOldObserver = Builder.setTemporaryChangeObserver(Observer);
 
   uint64_t CurrentIdx = 0;
   SmallVector OnFailResumeAt;
 
+  // We also record MachineInstrs manually in this vector so opcodes can address
+  // them.
+  SmallVector OutMIs;
+
   // Bypass the flag check on the instruction, and only look at the MCInstrDesc.
   bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();
 
@@ -71,19 +88,29 @@ bool GIMatchTableExecutor::executeMatchTable(
     return RejectAndResume;
   };
 
-  auto propagateFlags = [=](NewMIVector &OutMIs) {
-    for (auto MIB : OutMIs) {
+  auto propagateFlags = [&]() {
+    for (auto *MI : MTObserver.CreatedInsts) {
       // 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())
+      if (NoFPException && MI->mayRaiseFPException())
         MIBFlags |= MachineInstr::NoFPExcept;
-      MIB.setMIFlags(MIBFlags);
+      Observer.changingInstr(*MI);
+      MI->setFlags(MIBFlags);
+      Observer.changedInstr(*MI);
     }
 
     return true;
   };
 
+  // If the index is >= 0, it's an index in the type objects generated by TableGen.
+  // If the index is <0, it's an index in the recorded types object.
+  auto getTypeFromIdx = [&](int64_t Idx) -> const LLT& {
+    if(Idx >= 0)
+      return ExecInfo.TypeObjects[Idx];
+    return State.RecordedTypes[1 - Idx];
+  };
+
   while (true) {
     assert(CurrentIdx != ~0u && "Invalid MatchTable index");
     int64_t MatcherOpcode = MatchTable[CurrentIdx++];
@@ -620,7 +647,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
       MachineOperand &MO = State.MIs[InsnID]->getOperand(OpIdx);
       if (!MO.isReg() ||
-          MRI.getType(MO.getReg()) != ExecInfo.TypeObjects[TypeID]) {
+          MRI.getType(MO.getReg()) != getTypeFromIdx(TypeID)) {
         if (handleReject() == RejectAndGiveUp)
           return false;
       }
@@ -671,6 +698,25 @@ bool GIMatchTableExecutor::executeMatchTable(
       State.RecordedOperands[StoreIdx] = &State.MIs[InsnID]->getOperand(OpIdx);
       break;
     }
+    case GIM_RecordRegType: {
+      int64_t InsnID = MatchTable[CurrentIdx++];
+      int64_t OpIdx = MatchTable[CurrentIdx++];
+      int64_t TypeIdx = MatchTable[CurrentIdx++];
+
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIM_RecordRegType(MIs["
+                             << InsnID << "]->getOperand(" << OpIdx
+                             << "), TypeIdx=" << TypeIdx << ")\n");
+      assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+      assert(TypeIdx <= 0 && "Temp types always have negative indexes!");
+      // Indexes start at -1.
+      TypeIdx = 1 - TypeIdx;
+      const auto& Op = State.MIs[InsnID]->getOperand(OpIdx);
+      if(State.RecordedTypes.size() <= (uint64_t)TypeIdx)
+        State.RecordedTypes.resize(TypeIdx + 1, LLT());
+      State.RecordedTypes[TypeIdx] = MRI.getType(Op.getReg());
+      break;
+    }
     case GIM_CheckRegBankForClass: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t OpIdx = MatchTable[CurrentIdx++];
@@ -901,6 +947,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       OutMIs[NewInsnID] = MachineInstrBuilder(*State.MIs[OldInsnID]->getMF(),
                                               State.MIs[OldInsnID]);
       OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
+      MTObserver.CreatedInsts.insert(OutMIs[NewInsnID]);
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
                              << NewInsnID << "], MIs[" << OldInsnID << "], "
@@ -914,14 +961,23 @@ 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");
       break;
     }
 
+    case GIR_BuildConstant: {
+      int64_t TempRegID = MatchTable[CurrentIdx++];
+      int64_t Imm = MatchTable[CurrentIdx++];
+      Builder.buildConstant(State.TempRegisters[TempRegID], Imm);
+      DEBUG_WITH_TYPE(TgtExecutor::getName(),
+                      dbgs() << CurrentIdx << ": GIR_BuildConstant(TempReg["
+                             << TempRegID << "], Imm=" << Imm << ")\n");
+      break;
+    }
+
     case GIR_Copy: {
       int64_t NewInsnID = MatchTable[CurrentIdx++];
       int64_t OldInsnID = MatchTable[CurrentIdx++];
@@ -1047,24 +1103,6 @@ bool GIMatchTableExecutor::executeMatchTable(
                              << "], " << Imm << ")\n");
       break;
     }
-
-    case GIR_AddCImm: {
-      int64_t InsnID = MatchTable[CurrentIdx++];
-      int64_t TypeID = MatchTable[CurrentIdx++];
-      int64_t Imm = MatchTable[CurrentIdx++];
-      assert(OutMIs[InsnID] && "Attempted to add to undefined instruction");
-
-      unsigned Width = ExecInfo.TypeObjects[TypeID].getScalarSizeInBits();
-      LLVMContext &Ctx = MF->getFunction().getContext();
-      OutMIs[InsnID].addCImm(
-          ConstantInt::get(IntegerType::get(Ctx, Width), Imm, /*signed*/ true));
-      DEBUG_WITH_TYPE(TgtExecutor::getName(),
-                      dbgs() << CurrentIdx << ": GIR_AddCImm(OutMIs[" << InsnID
-                             << "], TypeID=" << TypeID << ", Imm=" << Imm
-                             << ")\n");
-      break;
-    }
-
     case GIR_ComplexRenderer: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t RendererID = MatchTable[CurrentIdx++];
@@ -1239,8 +1277,11 @@ bool GIMatchTableExecutor::executeMatchTable(
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs["
                              << InsnID << "])\n");
-      if (Observer)
-        Observer->erasingInstr(*MI);
+      // 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());
+      Observer.erasingInstr(*MI);
       MI->eraseFromParent();
       break;
     }
@@ -1250,7 +1291,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       int64_t TypeID = MatchTable[CurrentIdx++];
 
       State.TempRegisters[TempRegID] =
-          MRI.createGenericVirtualRegister(ExecInfo.TypeObjects[TypeID]);
+          MRI.createGenericVirtualRegister(getTypeFromIdx(TypeID));
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": TempRegs[" << TempRegID
                              << "] = GIR_MakeTempReg(" << TypeID << ")\n");
@@ -1269,11 +1310,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.MIs[NewInsnID]->getOperand(NewOpIdx).getReg();
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_ReplaceRegWithTempReg: {
@@ -1288,11 +1327,9 @@ bool GIMatchTableExecutor::executeMatchTable(
 
       Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
       Register New = State.TempRegisters[TempRegID];
-      if (Observer)
-        Observer->changingAllUsesOfReg(MRI, Old);
+      Observer.changingAllUsesOfReg(MRI, Old);
       MRI.replaceRegWith(Old, New);
-      if (Observer)
-        Observer->finishedChangingAllUsesOfReg();
+      Observer.finishedChangingAllUsesOfReg();
       break;
     }
     case GIR_Coverage: {
@@ -1309,11 +1346,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/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index e7db9547f03b694..99b821406b0f893 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -20,6 +20,7 @@
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/SaveAndRestore.h"
 
 namespace llvm {
 
@@ -364,6 +365,15 @@ class MachineIRBuilder {
     State.Observer = &Observer;
   }
 
+  GISelChangeObserver *getChangeObserver() const { return State.Observer; }
+
+  // Replaces the change observer with \p Observer and returns an object that
+  // restores the old Observer on destruction.
+  SaveAndRestore
+  setTemporaryChangeObserver(GISelChangeObserver &Observer) {
+    return SaveAndRestore(State.Observer, &Observer);
+  }
+
   void stopObservingChanges() { State.Observer = nullptr; }
 
   bool isObservingChanges() const { return State.Observer != nullptr; }
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 7e0691e1ee95048..4a22dd61c6e9b04 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -110,6 +110,24 @@ class GICombinePatFrag alts> {
   list Alternatives = alts;
 }
 
+//===----------------------------------------------------------------------===//
+// Pattern Special Types
+//===----------------------------------------------------------------------===//
+
+class GISpecialType;
+
+// In an apply pattern, GITypeOf can be used to set the type of a ne...

@Pierre-vh Pierre-vh marked this pull request as draft September 12, 2023 11:34
@Pierre-vh Pierre-vh closed this Sep 12, 2023
@Pierre-vh Pierre-vh deleted the final-cst-improvements branch September 12, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants