Skip to content

Commit ec61840

Browse files
Pierre-vhPierre-vh
committed
[RFC][GlobalISel] Use Builders in MatchTable
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.
1 parent 8f54861 commit ec61840

15 files changed

+193
-149
lines changed

‎llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "llvm/ADT/Bitset.h"
1919
#include "llvm/ADT/DenseMap.h"
2020
#include "llvm/ADT/SmallVector.h"
21+
#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
2122
#include "llvm/CodeGen/GlobalISel/Utils.h"
2223
#include "llvm/CodeGen/LowLevelType.h"
2324
#include "llvm/CodeGen/MachineFunction.h"
@@ -40,6 +41,7 @@ class APInt;
4041
class APFloat;
4142
class GISelKnownBits;
4243
class MachineInstr;
44+
class MachineIRBuilder;
4345
class MachineInstrBuilder;
4446
class MachineFunction;
4547
class MachineOperand;
@@ -501,10 +503,25 @@ class GIMatchTableExecutor {
501503
}
502504

503505
protected:
506+
/// Observer used by \ref executeMatchTable to record all instructions created
507+
/// by the rule.
508+
class GIMatchTableObserver : public GISelChangeObserver {
509+
public:
510+
virtual ~GIMatchTableObserver();
511+
512+
void erasingInstr(MachineInstr &MI) override { CreatedInsts.erase(&MI); }
513+
void createdInstr(MachineInstr &MI) override { CreatedInsts.insert(&MI); }
514+
void changingInstr(MachineInstr &MI) override {}
515+
void changedInstr(MachineInstr &MI) override {}
516+
517+
// Keeps track of all instructions that have been created when applying a
518+
// rule.
519+
SmallDenseSet<MachineInstr *, 4> CreatedInsts;
520+
};
521+
504522
using ComplexRendererFns =
505523
std::optional<SmallVector<std::function<void(MachineInstrBuilder &)>, 4>>;
506524
using RecordedMIVector = SmallVector<MachineInstr *, 4>;
507-
using NewMIVector = SmallVector<MachineInstrBuilder, 4>;
508525

509526
struct MatcherState {
510527
std::vector<ComplexRendererFns::value_type> Renderers;
@@ -555,15 +572,15 @@ class GIMatchTableExecutor {
555572
/// and false otherwise.
556573
template <class TgtExecutor, class PredicateBitset, class ComplexMatcherMemFn,
557574
class CustomRendererFn>
558-
bool executeMatchTable(
559-
TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
560-
const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn, CustomRendererFn>
561-
&ISelInfo,
562-
const int64_t *MatchTable, const TargetInstrInfo &TII,
563-
MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
564-
const RegisterBankInfo &RBI, const PredicateBitset &AvailableFeatures,
565-
CodeGenCoverage *CoverageInfo,
566-
GISelChangeObserver *Observer = nullptr) const;
575+
bool executeMatchTable(TgtExecutor &Exec, MatcherState &State,
576+
const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn,
577+
CustomRendererFn> &ExecInfo,
578+
MachineIRBuilder &Builder, const int64_t *MatchTable,
579+
const TargetInstrInfo &TII, MachineRegisterInfo &MRI,
580+
const TargetRegisterInfo &TRI,
581+
const RegisterBankInfo &RBI,
582+
const PredicateBitset &AvailableFeatures,
583+
CodeGenCoverage *CoverageInfo) const;
567584

568585
virtual const int64_t *getMatchTable() const {
569586
llvm_unreachable("Should have been overridden by tablegen if used");
@@ -592,7 +609,7 @@ class GIMatchTableExecutor {
592609
}
593610

594611
virtual void runCustomAction(unsigned, const MatcherState &State,
595-
NewMIVector &OutMIs) const {
612+
ArrayRef<MachineInstrBuilder> OutMIs) const {
596613
llvm_unreachable("Subclass does not implement runCustomAction!");
597614
}
598615

‎llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h

Lines changed: 40 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "llvm/ADT/SmallVector.h"
1919
#include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
2020
#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
21+
#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
2122
#include "llvm/CodeGen/GlobalISel/Utils.h"
2223
#include "llvm/CodeGen/MachineInstrBuilder.h"
2324
#include "llvm/CodeGen/MachineOperand.h"
@@ -42,17 +43,33 @@ namespace llvm {
4243
template <class TgtExecutor, class PredicateBitset, class ComplexMatcherMemFn,
4344
class CustomRendererFn>
4445
bool GIMatchTableExecutor::executeMatchTable(
45-
TgtExecutor &Exec, NewMIVector &OutMIs, MatcherState &State,
46+
TgtExecutor &Exec, MatcherState &State,
4647
const ExecInfoTy<PredicateBitset, ComplexMatcherMemFn, CustomRendererFn>
4748
&ExecInfo,
48-
const int64_t *MatchTable, const TargetInstrInfo &TII,
49-
MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
50-
const RegisterBankInfo &RBI, const PredicateBitset &AvailableFeatures,
51-
CodeGenCoverage *CoverageInfo, GISelChangeObserver *Observer) const {
49+
MachineIRBuilder &Builder, const int64_t *MatchTable,
50+
const TargetInstrInfo &TII, MachineRegisterInfo &MRI,
51+
const TargetRegisterInfo &TRI, const RegisterBankInfo &RBI,
52+
const PredicateBitset &AvailableFeatures,
53+
CodeGenCoverage *CoverageInfo) const {
54+
55+
// Setup observer
56+
GIMatchTableObserver MTObserver;
57+
GISelObserverWrapper Observer(&MTObserver);
58+
if (auto *CurObs = Builder.getObserver())
59+
Observer.addObserver(CurObs);
60+
61+
// TODO: Set MF delegate?
62+
63+
// Setup builder.
64+
auto RestoreOldObserver = Builder.setTemporaryChangeObserver(Observer);
5265

5366
uint64_t CurrentIdx = 0;
5467
SmallVector<uint64_t, 4> OnFailResumeAt;
5568

69+
// We also record MachineInstrs manually in this vector so opcodes can address
70+
// them.
71+
SmallVector<MachineInstrBuilder, 4> OutMIs;
72+
5673
// Bypass the flag check on the instruction, and only look at the MCInstrDesc.
5774
bool NoFPException = !State.MIs[0]->getDesc().mayRaiseFPException();
5875

@@ -71,14 +88,16 @@ bool GIMatchTableExecutor::executeMatchTable(
7188
return RejectAndResume;
7289
};
7390

74-
auto propagateFlags = [=](NewMIVector &OutMIs) {
75-
for (auto MIB : OutMIs) {
91+
auto propagateFlags = [&]() {
92+
for (auto *MI : MTObserver.CreatedInsts) {
7693
// Set the NoFPExcept flag when no original matched instruction could
7794
// raise an FP exception, but the new instruction potentially might.
7895
uint16_t MIBFlags = Flags;
79-
if (NoFPException && MIB->mayRaiseFPException())
96+
if (NoFPException && MI->mayRaiseFPException())
8097
MIBFlags |= MachineInstr::NoFPExcept;
81-
MIB.setMIFlags(MIBFlags);
98+
Observer.changingInstr(*MI);
99+
MI->setFlags(MIBFlags);
100+
Observer.changedInstr(*MI);
82101
}
83102

84103
return true;
@@ -901,6 +920,7 @@ bool GIMatchTableExecutor::executeMatchTable(
901920
OutMIs[NewInsnID] = MachineInstrBuilder(*State.MIs[OldInsnID]->getMF(),
902921
State.MIs[OldInsnID]);
903922
OutMIs[NewInsnID]->setDesc(TII.get(NewOpcode));
923+
MTObserver.CreatedInsts.insert(OutMIs[NewInsnID]);
904924
DEBUG_WITH_TYPE(TgtExecutor::getName(),
905925
dbgs() << CurrentIdx << ": GIR_MutateOpcode(OutMIs["
906926
<< NewInsnID << "], MIs[" << OldInsnID << "], "
@@ -914,8 +934,7 @@ bool GIMatchTableExecutor::executeMatchTable(
914934
if (NewInsnID >= OutMIs.size())
915935
OutMIs.resize(NewInsnID + 1);
916936

917-
OutMIs[NewInsnID] = BuildMI(*State.MIs[0]->getParent(), State.MIs[0],
918-
MIMetadata(*State.MIs[0]), TII.get(Opcode));
937+
OutMIs[NewInsnID] = Builder.buildInstr(Opcode);
919938
DEBUG_WITH_TYPE(TgtExecutor::getName(),
920939
dbgs() << CurrentIdx << ": GIR_BuildMI(OutMIs["
921940
<< NewInsnID << "], " << Opcode << ")\n");
@@ -1239,8 +1258,11 @@ bool GIMatchTableExecutor::executeMatchTable(
12391258
DEBUG_WITH_TYPE(TgtExecutor::getName(),
12401259
dbgs() << CurrentIdx << ": GIR_EraseFromParent(MIs["
12411260
<< InsnID << "])\n");
1242-
if (Observer)
1243-
Observer->erasingInstr(*MI);
1261+
// If we're erasing the insertion point, ensure we don't leave a dangling
1262+
// pointer in the builder.
1263+
if (Builder.getInsertPt() == MI)
1264+
Builder.setInsertPt(*MI->getParent(), ++MI->getIterator());
1265+
Observer.erasingInstr(*MI);
12441266
MI->eraseFromParent();
12451267
break;
12461268
}
@@ -1269,11 +1291,9 @@ bool GIMatchTableExecutor::executeMatchTable(
12691291

12701292
Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
12711293
Register New = State.MIs[NewInsnID]->getOperand(NewOpIdx).getReg();
1272-
if (Observer)
1273-
Observer->changingAllUsesOfReg(MRI, Old);
1294+
Observer.changingAllUsesOfReg(MRI, Old);
12741295
MRI.replaceRegWith(Old, New);
1275-
if (Observer)
1276-
Observer->finishedChangingAllUsesOfReg();
1296+
Observer.finishedChangingAllUsesOfReg();
12771297
break;
12781298
}
12791299
case GIR_ReplaceRegWithTempReg: {
@@ -1288,11 +1308,9 @@ bool GIMatchTableExecutor::executeMatchTable(
12881308

12891309
Register Old = State.MIs[OldInsnID]->getOperand(OldOpIdx).getReg();
12901310
Register New = State.TempRegisters[TempRegID];
1291-
if (Observer)
1292-
Observer->changingAllUsesOfReg(MRI, Old);
1311+
Observer.changingAllUsesOfReg(MRI, Old);
12931312
MRI.replaceRegWith(Old, New);
1294-
if (Observer)
1295-
Observer->finishedChangingAllUsesOfReg();
1313+
Observer.finishedChangingAllUsesOfReg();
12961314
break;
12971315
}
12981316
case GIR_Coverage: {
@@ -1309,11 +1327,7 @@ bool GIMatchTableExecutor::executeMatchTable(
13091327
case GIR_Done:
13101328
DEBUG_WITH_TYPE(TgtExecutor::getName(),
13111329
dbgs() << CurrentIdx << ": GIR_Done\n");
1312-
if (Observer) {
1313-
for (MachineInstr *MI : OutMIs)
1314-
Observer->createdInstr(*MI);
1315-
}
1316-
propagateFlags(OutMIs);
1330+
propagateFlags();
13171331
return true;
13181332
default:
13191333
llvm_unreachable("Unexpected command");

‎llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "llvm/CodeGen/TargetOpcodes.h"
2121
#include "llvm/IR/DebugLoc.h"
2222
#include "llvm/IR/Module.h"
23+
#include "llvm/Support/SaveAndRestore.h"
2324

2425
namespace llvm {
2526

@@ -364,6 +365,13 @@ class MachineIRBuilder {
364365
State.Observer = &Observer;
365366
}
366367

368+
// Replaces the change observer with \p Observer and returns an object that
369+
// restores the old Observer on destruction.
370+
SaveAndRestore<GISelChangeObserver *>
371+
setTemporaryChangeObserver(GISelChangeObserver &Observer) {
372+
return SaveAndRestore<GISelChangeObserver *>(State.Observer, &Observer);
373+
}
374+
367375
GISelChangeObserver *getObserver() { return State.Observer; }
368376

369377
void stopObservingChanges() { State.Observer = nullptr; }

‎llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@
2121

2222
using namespace llvm;
2323

24+
GIMatchTableExecutor::GIMatchTableObserver::~GIMatchTableObserver() {
25+
// anchor
26+
}
27+
2428
GIMatchTableExecutor::MatcherState::MatcherState(unsigned MaxRenderers)
2529
: Renderers(MaxRenderers) {}
2630

‎llvm/test/CodeGen/AArch64/GlobalISel/combine-sdiv.mir

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ body: |
2222
; CHECK: liveins: $w0
2323
; CHECK-NEXT: {{ $}}
2424
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
25-
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
26-
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
25+
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
26+
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
2727
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(s32) = exact G_ASHR [[COPY]], [[C]](s32)
28-
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[ASHR]], [[C1]]
28+
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(s32) = exact G_MUL [[ASHR]], [[C1]]
2929
; CHECK-NEXT: $w0 = COPY [[MUL]](s32)
3030
; CHECK-NEXT: RET_ReallyLR implicit $w0
3131
%0:_(s32) = COPY $w0
@@ -87,13 +87,13 @@ body: |
8787
; CHECK: liveins: $q0
8888
; CHECK-NEXT: {{ $}}
8989
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $q0
90-
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
91-
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
92-
; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 954437177
93-
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
94-
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C1]](s32), [[C2]](s32), [[C1]](s32), [[C2]](s32)
90+
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
91+
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
92+
; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 954437177
93+
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
94+
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C1]](s32), [[C2]](s32), [[C1]](s32), [[C2]](s32)
9595
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(<4 x s32>) = exact G_ASHR [[COPY]], [[BUILD_VECTOR]](<4 x s32>)
96-
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = G_MUL [[ASHR]], [[BUILD_VECTOR1]]
96+
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = exact G_MUL [[ASHR]], [[BUILD_VECTOR1]]
9797
; CHECK-NEXT: $q0 = COPY [[MUL]](<4 x s32>)
9898
; CHECK-NEXT: RET_ReallyLR implicit $q0
9999
%0:_(<4 x s32>) = COPY $q0
@@ -115,12 +115,12 @@ body: |
115115
; CHECK: liveins: $q0
116116
; CHECK-NEXT: {{ $}}
117117
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $q0
118-
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
119-
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
120-
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
121-
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C1]](s32), [[C1]](s32), [[C1]](s32), [[C1]](s32)
118+
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
119+
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
120+
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
121+
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C1]](s32), [[C1]](s32), [[C1]](s32), [[C1]](s32)
122122
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(<4 x s32>) = exact G_ASHR [[COPY]], [[BUILD_VECTOR]](<4 x s32>)
123-
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = G_MUL [[ASHR]], [[BUILD_VECTOR1]]
123+
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = exact G_MUL [[ASHR]], [[BUILD_VECTOR1]]
124124
; CHECK-NEXT: $q0 = COPY [[MUL]](<4 x s32>)
125125
; CHECK-NEXT: RET_ReallyLR implicit $q0
126126
%0:_(<4 x s32>) = COPY $q0

‎llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-ashr-shl-to-sext-inreg.mir

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ body: |
1515
; CHECK-NEXT: {{ $}}
1616
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
1717
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY]](s32)
18-
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(s16) = G_SEXT_INREG [[TRUNC]], 8
18+
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(s16) = exact G_SEXT_INREG [[TRUNC]], 8
1919
; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[SEXT_INREG]](s16)
2020
; CHECK-NEXT: $w0 = COPY [[ANYEXT]](s32)
2121
; CHECK-NEXT: RET_ReallyLR implicit $w0
@@ -75,7 +75,7 @@ body: |
7575
; CHECK: liveins: $d0
7676
; CHECK-NEXT: {{ $}}
7777
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s16>) = COPY $d0
78-
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(<4 x s16>) = G_SEXT_INREG [[COPY]], 8
78+
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(<4 x s16>) = exact G_SEXT_INREG [[COPY]], 8
7979
; CHECK-NEXT: $d0 = COPY [[SEXT_INREG]](<4 x s16>)
8080
; CHECK-NEXT: RET_ReallyLR implicit $d0
8181
%0:_(<4 x s16>) = COPY $d0

0 commit comments

Comments
 (0)