Skip to content

Commit

Permalink
[RFC][GlobalISel] Use Builders in MatchTable
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Pierre-vh
Pierre-vh committed Sep 12, 2023
1 parent 2126a18 commit facbc26
Show file tree
Hide file tree
Showing 15 changed files with 195 additions and 149 deletions.
39 changes: 28 additions & 11 deletions llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -40,6 +41,7 @@ class APInt;
class APFloat;
class GISelKnownBits;
class MachineInstr;
class MachineIRBuilder;
class MachineInstrBuilder;
class MachineFunction;
class MachineOperand;
Expand Down Expand Up @@ -501,10 +503,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<MachineInstr *, 4> CreatedInsts;
};

using ComplexRendererFns =
std::optional<SmallVector<std::function<void(MachineInstrBuilder &)>, 4>>;
using RecordedMIVector = SmallVector<MachineInstr *, 4>;
using NewMIVector = SmallVector<MachineInstrBuilder, 4>;

struct MatcherState {
std::vector<ComplexRendererFns::value_type> Renderers;
Expand Down Expand Up @@ -555,15 +572,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");
Expand Down Expand Up @@ -592,7 +609,7 @@ class GIMatchTableExecutor {
}

virtual void runCustomAction(unsigned, const MatcherState &State,
NewMIVector &OutMIs) const {
ArrayRef<MachineInstrBuilder> OutMIs) const {
llvm_unreachable("Subclass does not implement runCustomAction!");
}

Expand Down
66 changes: 40 additions & 26 deletions llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -42,17 +43,33 @@ 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 {

// 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<uint64_t, 4> OnFailResumeAt;

// We also record MachineInstrs manually in this vector so opcodes can address
// them.
SmallVector<MachineInstrBuilder, 4> OutMIs;

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

Expand All @@ -71,14 +88,16 @@ 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;
Expand Down Expand Up @@ -901,6 +920,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 << "], "
Expand All @@ -914,8 +934,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");
Expand Down Expand Up @@ -1239,8 +1258,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;
}
Expand Down Expand Up @@ -1269,11 +1291,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: {
Expand All @@ -1288,11 +1308,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: {
Expand All @@ -1309,11 +1327,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");
Expand Down
10 changes: 10 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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<GISelChangeObserver *>
setTemporaryChangeObserver(GISelChangeObserver &Observer) {
return SaveAndRestore<GISelChangeObserver *>(State.Observer, &Observer);
}

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

bool isObservingChanges() const { return State.Observer != nullptr; }
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@

using namespace llvm;

GIMatchTableExecutor::GIMatchTableObserver::~GIMatchTableObserver() {
// anchor
}

GIMatchTableExecutor::MatcherState::MatcherState(unsigned MaxRenderers)
: Renderers(MaxRenderers) {}

Expand Down
28 changes: 14 additions & 14 deletions llvm/test/CodeGen/AArch64/GlobalISel/combine-sdiv.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ body: |
; CHECK: liveins: $w0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(s32) = exact G_ASHR [[COPY]], [[C]](s32)
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[ASHR]], [[C1]]
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(s32) = exact G_MUL [[ASHR]], [[C1]]
; CHECK-NEXT: $w0 = COPY [[MUL]](s32)
; CHECK-NEXT: RET_ReallyLR implicit $w0
%0:_(s32) = COPY $w0
Expand Down Expand Up @@ -87,13 +87,13 @@ body: |
; CHECK: liveins: $q0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $q0
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 954437177
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C1]](s32), [[C2]](s32), [[C1]](s32), [[C2]](s32)
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 954437177
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C1]](s32), [[C2]](s32), [[C1]](s32), [[C2]](s32)
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(<4 x s32>) = exact G_ASHR [[COPY]], [[BUILD_VECTOR]](<4 x s32>)
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = G_MUL [[ASHR]], [[BUILD_VECTOR1]]
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = exact G_MUL [[ASHR]], [[BUILD_VECTOR1]]
; CHECK-NEXT: $q0 = COPY [[MUL]](<4 x s32>)
; CHECK-NEXT: RET_ReallyLR implicit $q0
%0:_(<4 x s32>) = COPY $q0
Expand All @@ -115,12 +115,12 @@ body: |
; CHECK: liveins: $q0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s32>) = COPY $q0
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 -991146299
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = G_BUILD_VECTOR [[C1]](s32), [[C1]](s32), [[C1]](s32), [[C1]](s32)
; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 3
; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = exact G_CONSTANT i32 -991146299
; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C]](s32), [[C]](s32), [[C]](s32), [[C]](s32)
; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s32>) = exact G_BUILD_VECTOR [[C1]](s32), [[C1]](s32), [[C1]](s32), [[C1]](s32)
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(<4 x s32>) = exact G_ASHR [[COPY]], [[BUILD_VECTOR]](<4 x s32>)
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = G_MUL [[ASHR]], [[BUILD_VECTOR1]]
; CHECK-NEXT: [[MUL:%[0-9]+]]:_(<4 x s32>) = exact G_MUL [[ASHR]], [[BUILD_VECTOR1]]
; CHECK-NEXT: $q0 = COPY [[MUL]](<4 x s32>)
; CHECK-NEXT: RET_ReallyLR implicit $q0
%0:_(<4 x s32>) = COPY $q0
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-ashr-shl-to-sext-inreg.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ body: |
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY]](s32)
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(s16) = G_SEXT_INREG [[TRUNC]], 8
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(s16) = exact G_SEXT_INREG [[TRUNC]], 8
; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[SEXT_INREG]](s16)
; CHECK-NEXT: $w0 = COPY [[ANYEXT]](s32)
; CHECK-NEXT: RET_ReallyLR implicit $w0
Expand Down Expand Up @@ -75,7 +75,7 @@ body: |
; CHECK: liveins: $d0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(<4 x s16>) = COPY $d0
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(<4 x s16>) = G_SEXT_INREG [[COPY]], 8
; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(<4 x s16>) = exact G_SEXT_INREG [[COPY]], 8
; CHECK-NEXT: $d0 = COPY [[SEXT_INREG]](<4 x s16>)
; CHECK-NEXT: RET_ReallyLR implicit $d0
%0:_(<4 x s16>) = COPY $d0
Expand Down
Loading

0 comments on commit facbc26

Please sign in to comment.