InstructionSelect: Use GISelChangeObserver instead of MachineFunction::Delegate #105725
Conversation
|
@llvm/pr-subscribers-llvm-globalisel Author: Daniel Sanders (dsandersllvm) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/105725.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
index 7ec5dac9a6ebaf..214777ca9082d8 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
@@ -138,5 +138,20 @@ 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
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
index aaba56ee11251c..ae75f2593aeffe 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
@@ -16,6 +16,8 @@
#include "llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h"
namespace llvm {
+class GISelObserverWrapper;
+
class InstructionSelector : public GIMatchTableExecutor {
public:
virtual ~InstructionSelector();
@@ -36,6 +38,8 @@ class InstructionSelector : public GIMatchTableExecutor {
const TargetPassConfig *TPC = nullptr;
MachineOptimizationRemarkEmitter *MORE = nullptr;
+
+ GISelObserverWrapper *AllObservers = nullptr;
};
} // namespace llvm
diff --git a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
index 59f4d60a41d80d..836d54fa989d78 100644
--- a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
@@ -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);
+}
diff --git a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
index 8c0bb85fd0771c..f89a0e84bcd420 100644
--- a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
@@ -76,18 +76,21 @@ InstructionSelect::InstructionSelect(CodeGenOptLevel OL, char &PassID)
/// deletion of arbitrary instructions, we detect this case and continue
/// selection with the predecessor of the deleted instruction.
class InstructionSelect::MIIteratorMaintainer
- : public MachineFunction::Delegate {
+ : 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 {}
+ void changedInstr(MachineInstr &MI) override {}
+
+ 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
@@ -190,8 +193,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;
@@ -215,6 +221,7 @@ bool InstructionSelect::selectMachineFunction(MachineFunction &MF) {
LLVM_DEBUG(MIIMaintainer.reportFullyCreatedInstrs());
}
}
+ AllObservers.removeObserver(&MIIMaintainer);
}
for (MachineBasicBlock &MBB : MF) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…chineFunction::Delegate 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.
dsandersllvm
force-pushed
the
delegate-to-changeobserver
branch
from
13cdc0c to
3205d6f
Compare
There was a problem hiding this comment.
(For future reference: This causes a 0.05% geomean compile-time (instruction count) regression on CTMark AArch64 O0, which isn't negligible. This was the original reasoning for not using the ObserverWrapper. This is a more general issue with the ObserverWrapper. I eventually want to rework this such that when the ObserverWrapper contains only one Observer, it is installed without indirection to avoid this overhead.)
I'm fine with this change though.
…d of MachineFunction::Delegate
|
Those changes are all done. I remember there was a conversation on discourse a few months back about clicking the resolve buttons but I'm not actually sure what the outcome was. I saw you were against the ban so I've resolved them
Thanks
Our downstream usage of the delegate is quite localized to a couple intrinsics so hopefully we can remove it. If we manage that I'll come back to revert this commit |
There was a problem hiding this comment.
Those changes are all done. I remember there was a conversation on discourse a few months back about clicking the resolve buttons but I'm not actually sure what the outcome was. I saw you were against the ban so I've resolved them
Thanks. You might have mistaken me for a different Tobias though, I am a relatively new contributor. No worries though.
Our downstream usage of the delegate is quite localized to a couple intrinsics so hopefully we can remove it. If we manage that I'll come back to revert this commit
I'd be fine with this either way. The ObserverWrapper issue needs to be fixed globally anyways, so I don't see a reason for you being blocked on this for now. I also think this is a valid usecase. I actually have a patch lying around for allowing reselection of generic instructions natively in InstructionSelect. I am investigating this because ideally I'd want to get rid of AArch64PostLegalizerLowering and Amara has also pointed out that this might be useful for simplifying preISelLower transformations in the AArch64InstructionSelector. I am just not happy enough with my approach yet for an RFC.
LGTM, but maybe give others a chance to chime in if this isn't urgent.
Ah yep, checking back on it I see that it was Tobias Shieta. Sorry about that |
…::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.
…::Delegate (#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.
…::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.
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.