[RFC][GlobalISel] Use Builders in MatchTable

[Pierre-vh]

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.

[arsenm]

Currently you're only supposed to push one patch per pull request, you can just pre-push the test regeneration

[arsenm]

I'm not really a fan of observers. Why does the match table need these? The execution should know precisely what it's doing without tracking other dynamic context?

[Pierre-vh]

I'm not really a fan of observers. Why does the match table need these? The execution should know precisely what it's doing without tracking other dynamic context?

We still use C++ most of the time, and it's impossible for the MatchTable to know what's going on in there unless it uses an observer, so I thought it'd be a nice change to use an observer for tracking all changes.
Likewise, when we use builder functions, like in #66077, we have no way of knowing how many insts were actually built without using a builder.

We can do without the builder, but then flag propagation will be a bit broken.

[qcolombet]

I think it makes sense to use MachineInstrBuilder in general.
Regarding Observes, I think it makes sense (maybe?) to use them to manage the worklist of things that needs to be combined/selected, but it is a no-no IMHO for flag propagation.

[Pierre-vh]

I reverted the observer changes, so now it's just adding the MachineIRBuilder and nothing else changes pretty much.
It's all I need to implement the other patches in this series

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff fe7fe6d3437da98017b93c99737e421c35e52b7b 06e1fb6ea742d86c50ce410a235893df09e07f48 -- llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp llvm/utils/TableGen/GlobalISelEmitter.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
index 1a0a0f46cc73..6f0f9a6a46c7 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
@@ -52,12 +52,11 @@ bool GIMatchTableExecutor::executeMatchTable(
     const PredicateBitset &AvailableFeatures,
     CodeGenCoverage *CoverageInfo) const {
 
-
   uint64_t CurrentIdx = 0;
   SmallVector<uint64_t, 4> OnFailResumeAt;
   NewMIVector OutMIs;
 
-  GISelChangeObserver* Observer = Builder.getObserver();
+  GISelChangeObserver *Observer = Builder.getObserver();
   // Bypass the flag check on the instruction, and only look at the MCInstrDesc.
   bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();
 
@@ -907,13 +906,12 @@ bool GIMatchTableExecutor::executeMatchTable(
       if (NewInsnID >= OutMIs.size())
         OutMIs.resize(NewInsnID + 1);
 
-      MachineInstr* OldMI = State.MIs[OldInsnID];
-      if(Observer)
+      MachineInstr *OldMI = State.MIs[OldInsnID];
+      if (Observer)
         Observer->changingInstr(*OldMI);
-      OutMIs[NewInsnID] = MachineInstrBuilder(*OldMI->getMF(),
-                                              OldMI);
+      OutMIs[NewInsnID] = MachineInstrBuilder(*OldMI->getMF(), OldMI);
       OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
-      if(Observer)
+      if (Observer)
         Observer->changedInstr(*OldMI);
       DEBUG_WITH_TYPE(TgtExecutor::getName(),
                       dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
@@ -1256,7 +1254,7 @@ bool GIMatchTableExecutor::executeMatchTable(
       // pointer in the builder.
       if (Builder.getInsertPt() == MI)
         Builder.setInsertPt(*MI->getParent(), ++MI->getIterator());
-      if(Observer)
+      if (Observer)
         Observer->erasingInstr(*MI);
       MI->eraseFromParent();
       break;

[Pierre-vh]

Ping :)

[jayfoad]

Looks reasonable to me.

[Pierre-vh]

@qcolombet can this land? Thanks

[qcolombet]

@qcolombet can this land? Thanks

Yep, looks fine