Skip to content

Commit

Permalink
InstructionSelect: Use GISelChangeObserver instead of MachineFunction…
Browse files Browse the repository at this point in the history
…::Delegate (llvm#105725)

The main difference is that it's possible for multiple change observers
to be installed at the same time whereas there can only be one
MachineFunction delegate installed. This allows downstream targets to
continue to use observers to recursively select. The target in question
was selecting a gMIR instruction to a machine instruction plus some gMIR
around it and relying on observers to ensure it correctly selected any
gMIR it created before returning to the main loop.
  • Loading branch information
dsandersllvm authored Aug 23, 2024
1 parent 3d18cea commit 0bf5846
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 5 deletions.
14 changes: 14 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,19 @@ class RAIIMFObsDelInstaller {
~RAIIMFObsDelInstaller() = default;
};

/// A simple RAII based Observer installer.
/// Use this in a scope to install the Observer to the MachineFunction and reset
/// it at the end of the scope.
class RAIITemporaryObserverInstaller {
public:
RAIITemporaryObserverInstaller(GISelObserverWrapper &Observers,
GISelChangeObserver &TemporaryObserver);
~RAIITemporaryObserverInstaller();

private:
GISelObserverWrapper &Observers;
GISelChangeObserver &TemporaryObserver;
};

} // namespace llvm
#endif
7 changes: 7 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"

namespace llvm {
class GISelObserverWrapper;

class InstructionSelector : public GIMatchTableExecutor {
public:
virtual ~InstructionSelector();
Expand All @@ -36,6 +38,11 @@ class InstructionSelector : public GIMatchTableExecutor {
const TargetPassConfig *TPC = nullptr;

MachineOptimizationRemarkEmitter *MORE = nullptr;

/// Note: InstructionSelect does not track changed instructions.
/// changingInstr() and changedInstr() will never be called on these
/// observers.
GISelObserverWrapper *AllObservers = nullptr;
};
} // namespace llvm

Expand Down
10 changes: 10 additions & 0 deletions llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,13 @@ RAIIMFObserverInstaller::RAIIMFObserverInstaller(MachineFunction &MF,
}

RAIIMFObserverInstaller::~RAIIMFObserverInstaller() { MF.setObserver(nullptr); }

RAIITemporaryObserverInstaller::RAIITemporaryObserverInstaller(
GISelObserverWrapper &Observers, GISelChangeObserver &TemporaryObserver)
: Observers(Observers), TemporaryObserver(TemporaryObserver) {
Observers.addObserver(&TemporaryObserver);
}

RAIITemporaryObserverInstaller::~RAIITemporaryObserverInstaller() {
Observers.removeObserver(&TemporaryObserver);
}
19 changes: 14 additions & 5 deletions llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,25 @@ InstructionSelect::InstructionSelect(CodeGenOptLevel OL, char &PassID)
/// a non-obvious limitation for selector implementers. Therefore, to allow
/// deletion of arbitrary instructions, we detect this case and continue
/// selection with the predecessor of the deleted instruction.
class InstructionSelect::MIIteratorMaintainer
: public MachineFunction::Delegate {
class InstructionSelect::MIIteratorMaintainer : public GISelChangeObserver {
#ifndef NDEBUG
SmallSetVector<const MachineInstr *, 32> CreatedInstrs;
#endif
public:
MachineBasicBlock::reverse_iterator MII;

void MF_HandleInsertion(MachineInstr &MI) override {
void changingInstr(MachineInstr &MI) override {
llvm_unreachable("InstructionSelect does not track changed instructions!");
}
void changedInstr(MachineInstr &MI) override {
llvm_unreachable("InstructionSelect does not track changed instructions!");
}

void createdInstr(MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << "Creating: " << MI; CreatedInstrs.insert(&MI));
}

void MF_HandleRemoval(MachineInstr &MI) override {
void erasingInstr(MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << "Erasing: " << MI; CreatedInstrs.remove(&MI));
if (MII.getInstrIterator().getNodePtr() == &MI) {
// If the iterator points to the MI that will be erased (i.e. the MI prior
Expand Down Expand Up @@ -190,8 +196,11 @@ bool InstructionSelect::selectMachineFunction(MachineFunction &MF) {
// GISelChangeObserver, because we do not want notifications about changed
// instructions. This prevents significant compile-time regressions from
// e.g. constrainOperandRegClass().
GISelObserverWrapper AllObservers;
MIIteratorMaintainer MIIMaintainer;
RAIIDelegateInstaller DelInstaller(MF, &MIIMaintainer);
AllObservers.addObserver(&MIIMaintainer);
RAIIDelegateInstaller DelInstaller(MF, &AllObservers);
ISel->AllObservers = &AllObservers;

for (MachineBasicBlock *MBB : post_order(&MF)) {
ISel->CurMBB = MBB;
Expand Down

0 comments on commit 0bf5846

Please sign in to comment.