[llvm] d2336fd - [RFC][GlobalISel] InstructionSelect: Allow arbitrary instruction erasure (#97670)
via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 11 08:26:46 PDT 2024
Author: Tobias Stadler
Date: 2024-08-11T17:26:43+02:00
New Revision: d2336fd75cc93d5191d56f8c7486069dc8aeec5b
URL: https://github.com/llvm/llvm-project/commit/d2336fd75cc93d5191d56f8c7486069dc8aeec5b
DIFF: https://github.com/llvm/llvm-project/commit/d2336fd75cc93d5191d56f8c7486069dc8aeec5b.diff
LOG: [RFC][GlobalISel] InstructionSelect: Allow arbitrary instruction erasure (#97670)
See https://discourse.llvm.org/t/rfc-globalisel-instructionselect-allow-arbitrary-instruction-erasure
Added:
llvm/unittests/CodeGen/GlobalISel/InstructionSelectTest.cpp
Modified:
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelect.h
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
llvm/unittests/CodeGen/GlobalISel/CMakeLists.txt
Removed:
################################################################################
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelect.h b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelect.h
index 8017f09aa3c8b..a2f06e21a700e 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelect.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelect.h
@@ -20,6 +20,8 @@
namespace llvm {
+class InstructionSelector;
+class GISelKnownBits;
class BlockFrequencyInfo;
class ProfileSummaryInfo;
@@ -53,12 +55,20 @@ class InstructionSelect : public MachineFunctionPass {
char &PassID = ID);
bool runOnMachineFunction(MachineFunction &MF) override;
+ bool selectMachineFunction(MachineFunction &MF);
+ void setInstructionSelector(InstructionSelector *NewISel) { ISel = NewISel; }
protected:
+ class MIIteratorMaintainer;
+
+ InstructionSelector *ISel = nullptr;
+ GISelKnownBits *KB = nullptr;
BlockFrequencyInfo *BFI = nullptr;
ProfileSummaryInfo *PSI = nullptr;
CodeGenOptLevel OptLevel = CodeGenOptLevel::None;
+
+ bool selectInstr(MachineInstr &MI);
};
} // End namespace llvm.
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
index 8331cb58a0991..aaba56ee11251 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
@@ -32,12 +32,9 @@ class InstructionSelector : public GIMatchTableExecutor {
/// !isPreISelGenericOpcode(I.getOpcode())
virtual bool select(MachineInstr &I) = 0;
- void setTargetPassConfig(const TargetPassConfig *T) { TPC = T; }
-
- void setRemarkEmitter(MachineOptimizationRemarkEmitter *M) { MORE = M; }
-
-protected:
+ // FIXME: Eliminate dependency on TargetPassConfig for NewPM transition
const TargetPassConfig *TPC = nullptr;
+
MachineOptimizationRemarkEmitter *MORE = nullptr;
};
} // namespace llvm
diff --git a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
index 9a27728dcb4dd..8c0bb85fd0771 100644
--- a/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
@@ -12,8 +12,10 @@
#include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/SetVector.h"
#include "llvm/Analysis/LazyBlockFrequencyInfo.h"
#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
#include "llvm/CodeGen/GlobalISel/GISelKnownBits.h"
#include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
#include "llvm/CodeGen/GlobalISel/LegalizerInfo.h"
@@ -65,6 +67,52 @@ INITIALIZE_PASS_END(InstructionSelect, DEBUG_TYPE,
InstructionSelect::InstructionSelect(CodeGenOptLevel OL, char &PassID)
: MachineFunctionPass(PassID), OptLevel(OL) {}
+/// This class observes instruction insertions/removals.
+/// InstructionSelect stores an iterator of the instruction prior to the one
+/// that is currently being selected to determine which instruction to select
+/// next. Previously this meant that selecting multiple instructions at once was
+/// illegal behavior due to potential invalidation of this iterator. This is
+/// 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 {
+#ifndef NDEBUG
+ SmallSetVector<const MachineInstr *, 32> CreatedInstrs;
+#endif
+public:
+ MachineBasicBlock::reverse_iterator MII;
+
+ void MF_HandleInsertion(MachineInstr &MI) override {
+ LLVM_DEBUG(dbgs() << "Creating: " << MI; CreatedInstrs.insert(&MI));
+ }
+
+ void MF_HandleRemoval(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
+ // to the MI that is currently being selected), the iterator would be
+ // invalidated. Continue selection with its predecessor.
+ ++MII;
+ LLVM_DEBUG(dbgs() << "Instruction removal updated iterator.\n");
+ }
+ }
+
+ void reportFullyCreatedInstrs() {
+ LLVM_DEBUG({
+ if (CreatedInstrs.empty()) {
+ dbgs() << "Created no instructions.\n";
+ } else {
+ dbgs() << "Created:\n";
+ for (const auto *MI : CreatedInstrs) {
+ dbgs() << " " << *MI;
+ }
+ CreatedInstrs.clear();
+ }
+ });
+ }
+};
+
void InstructionSelect::getAnalysisUsage(AnalysisUsage &AU) const {
AU.addRequired<TargetPassConfig>();
AU.addRequired<GISelKnownBitsAnalysis>();
@@ -84,31 +132,36 @@ bool InstructionSelect::runOnMachineFunction(MachineFunction &MF) {
MachineFunctionProperties::Property::FailedISel))
return false;
- LLVM_DEBUG(dbgs() << "Selecting function: " << MF.getName() << '\n');
-
- const TargetPassConfig &TPC = getAnalysis<TargetPassConfig>();
- InstructionSelector *ISel = MF.getSubtarget().getInstructionSelector();
- ISel->setTargetPassConfig(&TPC);
+ ISel = MF.getSubtarget().getInstructionSelector();
+ ISel->TPC = &getAnalysis<TargetPassConfig>();
+ // FIXME: Properly override OptLevel in TargetMachine. See OptLevelChanger
CodeGenOptLevel OldOptLevel = OptLevel;
auto RestoreOptLevel = make_scope_exit([=]() { OptLevel = OldOptLevel; });
OptLevel = MF.getFunction().hasOptNone() ? CodeGenOptLevel::None
: MF.getTarget().getOptLevel();
- GISelKnownBits *KB = &getAnalysis<GISelKnownBitsAnalysis>().get(MF);
+ KB = &getAnalysis<GISelKnownBitsAnalysis>().get(MF);
if (OptLevel != CodeGenOptLevel::None) {
PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
if (PSI && PSI->hasProfileSummary())
BFI = &getAnalysis<LazyBlockFrequencyInfoPass>().getBFI();
}
- CodeGenCoverage CoverageInfo;
+ return selectMachineFunction(MF);
+}
+
+bool InstructionSelect::selectMachineFunction(MachineFunction &MF) {
+ LLVM_DEBUG(dbgs() << "Selecting function: " << MF.getName() << '\n');
assert(ISel && "Cannot work without InstructionSelector");
+
+ const TargetPassConfig &TPC = *ISel->TPC;
+ CodeGenCoverage CoverageInfo;
ISel->setupMF(MF, KB, &CoverageInfo, PSI, BFI);
// An optimization remark emitter. Used to report failures.
MachineOptimizationRemarkEmitter MORE(MF, /*MBFI=*/nullptr);
- ISel->setRemarkEmitter(&MORE);
+ ISel->MORE = &MORE;
// FIXME: There are many other MF/MFI fields we need to initialize.
@@ -131,80 +184,36 @@ bool InstructionSelect::runOnMachineFunction(MachineFunction &MF) {
// Keep track of selected blocks, so we can delete unreachable ones later.
DenseSet<MachineBasicBlock *> SelectedBlocks;
- for (MachineBasicBlock *MBB : post_order(&MF)) {
- ISel->CurMBB = MBB;
- SelectedBlocks.insert(MBB);
- if (MBB->empty())
- continue;
-
- // Select instructions in reverse block order. We permit erasing so have
- // to resort to manually iterating and recognizing the begin (rend) case.
- bool ReachedBegin = false;
- for (auto MII = std::prev(MBB->end()), Begin = MBB->begin();
- !ReachedBegin;) {
-#ifndef NDEBUG
- // Keep track of the insertion range for debug printing.
- const auto AfterIt = std::next(MII);
-#endif
- // Select this instruction.
- MachineInstr &MI = *MII;
-
- // And have our iterator point to the next instruction, if there is one.
- if (MII == Begin)
- ReachedBegin = true;
- else
- --MII;
-
- LLVM_DEBUG(dbgs() << "Selecting: \n " << MI);
-
- // We could have folded this instruction away already, making it dead.
- // If so, erase it.
- if (isTriviallyDead(MI, MRI)) {
- LLVM_DEBUG(dbgs() << "Is dead; erasing.\n");
- salvageDebugInfo(MRI, MI);
- MI.eraseFromParent();
- continue;
- }
-
- // Eliminate hints or G_CONSTANT_FOLD_BARRIER.
- if (isPreISelGenericOptimizationHint(MI.getOpcode()) ||
- MI.getOpcode() == TargetOpcode::G_CONSTANT_FOLD_BARRIER) {
- auto [DstReg, SrcReg] = MI.getFirst2Regs();
-
- // At this point, the destination register class of the op may have
- // been decided.
- //
- // Propagate that through to the source register.
- const TargetRegisterClass *DstRC = MRI.getRegClassOrNull(DstReg);
- if (DstRC)
- MRI.setRegClass(SrcReg, DstRC);
- assert(canReplaceReg(DstReg, SrcReg, MRI) &&
- "Must be able to replace dst with src!");
- MI.eraseFromParent();
- MRI.replaceRegWith(DstReg, SrcReg);
- continue;
- }
-
- if (MI.getOpcode() == TargetOpcode::G_INVOKE_REGION_START) {
- MI.eraseFromParent();
- continue;
- }
-
- if (!ISel->select(MI)) {
- // FIXME: It would be nice to dump all inserted instructions. It's
- // not obvious how, esp. considering select() can insert after MI.
- reportGISelFailure(MF, TPC, MORE, "gisel-select", "cannot select", MI);
- return false;
+ {
+ // Observe IR insertions and removals during selection.
+ // We only install a MachineFunction::Delegate instead of a
+ // GISelChangeObserver, because we do not want notifications about changed
+ // instructions. This prevents significant compile-time regressions from
+ // e.g. constrainOperandRegClass().
+ MIIteratorMaintainer MIIMaintainer;
+ RAIIDelegateInstaller DelInstaller(MF, &MIIMaintainer);
+
+ for (MachineBasicBlock *MBB : post_order(&MF)) {
+ ISel->CurMBB = MBB;
+ SelectedBlocks.insert(MBB);
+
+ // Select instructions in reverse block order.
+ MIIMaintainer.MII = MBB->rbegin();
+ for (auto End = MBB->rend(); MIIMaintainer.MII != End;) {
+ MachineInstr &MI = *MIIMaintainer.MII;
+ // Increment early to skip instructions inserted by select().
+ ++MIIMaintainer.MII;
+
+ LLVM_DEBUG(dbgs() << "\nSelect: " << MI);
+ if (!selectInstr(MI)) {
+ LLVM_DEBUG(dbgs() << "Selection failed!\n";
+ MIIMaintainer.reportFullyCreatedInstrs());
+ reportGISelFailure(MF, TPC, MORE, "gisel-select", "cannot select",
+ MI);
+ return false;
+ }
+ LLVM_DEBUG(MIIMaintainer.reportFullyCreatedInstrs());
}
-
- // Dump the range of instructions that MI expanded into.
- LLVM_DEBUG({
- auto InsertedBegin = ReachedBegin ? MBB->begin() : std::next(MII);
- dbgs() << "Into:\n";
- for (auto &InsertedMI : make_range(InsertedBegin, AfterIt))
- dbgs() << " " << InsertedMI;
- dbgs() << '\n';
- });
}
}
@@ -222,16 +231,10 @@ bool InstructionSelect::runOnMachineFunction(MachineFunction &MF) {
continue;
}
// Try to find redundant copies b/w vregs of the same register class.
- bool ReachedBegin = false;
- for (auto MII = std::prev(MBB.end()), Begin = MBB.begin(); !ReachedBegin;) {
- // Select this instruction.
+ for (auto MII = MBB.rbegin(), End = MBB.rend(); MII != End;) {
MachineInstr &MI = *MII;
+ ++MII;
- // And have our iterator point to the next instruction, if there is one.
- if (MII == Begin)
- ReachedBegin = true;
- else
- --MII;
if (MI.getOpcode() != TargetOpcode::COPY)
continue;
Register SrcReg = MI.getOperand(1).getReg();
@@ -336,3 +339,42 @@ bool InstructionSelect::runOnMachineFunction(MachineFunction &MF) {
// FIXME: Should we accurately track changes?
return true;
}
+
+bool InstructionSelect::selectInstr(MachineInstr &MI) {
+ MachineRegisterInfo &MRI = ISel->MF->getRegInfo();
+
+ // We could have folded this instruction away already, making it dead.
+ // If so, erase it.
+ if (isTriviallyDead(MI, MRI)) {
+ LLVM_DEBUG(dbgs() << "Is dead.\n");
+ salvageDebugInfo(MRI, MI);
+ MI.eraseFromParent();
+ return true;
+ }
+
+ // Eliminate hints or G_CONSTANT_FOLD_BARRIER.
+ if (isPreISelGenericOptimizationHint(MI.getOpcode()) ||
+ MI.getOpcode() == TargetOpcode::G_CONSTANT_FOLD_BARRIER) {
+ auto [DstReg, SrcReg] = MI.getFirst2Regs();
+
+ // At this point, the destination register class of the op may have
+ // been decided.
+ //
+ // Propagate that through to the source register.
+ const TargetRegisterClass *DstRC = MRI.getRegClassOrNull(DstReg);
+ if (DstRC)
+ MRI.setRegClass(SrcReg, DstRC);
+ assert(canReplaceReg(DstReg, SrcReg, MRI) &&
+ "Must be able to replace dst with src!");
+ MI.eraseFromParent();
+ MRI.replaceRegWith(DstReg, SrcReg);
+ return true;
+ }
+
+ if (MI.getOpcode() == TargetOpcode::G_INVOKE_REGION_START) {
+ MI.eraseFromParent();
+ return true;
+ }
+
+ return ISel->select(MI);
+}
diff --git a/llvm/unittests/CodeGen/GlobalISel/CMakeLists.txt b/llvm/unittests/CodeGen/GlobalISel/CMakeLists.txt
index 253b90d5dc1f8..bce91c1ed6173 100644
--- a/llvm/unittests/CodeGen/GlobalISel/CMakeLists.txt
+++ b/llvm/unittests/CodeGen/GlobalISel/CMakeLists.txt
@@ -28,4 +28,5 @@ add_llvm_unittest(GlobalISelTests
GISelUtilsTest.cpp
GISelAliasTest.cpp
CallLowering.cpp
+ InstructionSelectTest.cpp
)
diff --git a/llvm/unittests/CodeGen/GlobalISel/InstructionSelectTest.cpp b/llvm/unittests/CodeGen/GlobalISel/InstructionSelectTest.cpp
new file mode 100644
index 0000000000000..7fbccf7160e17
--- /dev/null
+++ b/llvm/unittests/CodeGen/GlobalISel/InstructionSelectTest.cpp
@@ -0,0 +1,76 @@
+#include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
+#include "GISelMITest.h"
+#include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
+#include "llvm/IR/LegacyPassManager.h"
+
+namespace {
+
+class EraseMockInstructionSelector : public InstructionSelector {
+public:
+ int NumSelected = 0;
+ SmallVector<MachineInstr *> MIs;
+
+ bool select(MachineInstr &MI) override {
+ ++NumSelected;
+ switch (NumSelected) {
+ case 1:
+ EXPECT_EQ(&MI, MIs[8]);
+ // Erase previous instructions
+ MIs[7]->eraseFromParent();
+ MIs[6]->eraseFromParent();
+ // Don't erase this MI before step 3 to prevent DCE
+ return true;
+ case 2:
+ EXPECT_EQ(&MI, MIs[5]);
+ // Erase previous instructions reversed
+ MIs[3]->eraseFromParent();
+ MIs[4]->eraseFromParent();
+ MI.eraseFromParent();
+ return true;
+ case 3:
+ EXPECT_EQ(&MI, MIs[2]);
+ MIs[8]->eraseFromParent();
+ // Erase first instructions
+ MIs[0]->eraseFromParent();
+ MIs[1]->eraseFromParent();
+ MI.eraseFromParent();
+ return true;
+ default:
+ ADD_FAILURE();
+ return false;
+ }
+ }
+
+ void setupGeneratedPerFunctionState(MachineFunction &MF) override {}
+};
+
+TEST_F(AArch64GISelMITest, TestInstructionSelectErase) {
+ StringRef MIRString = R"(
+ $x0 = COPY %2(s64)
+ $x0 = COPY %2(s64)
+ $x0 = COPY %2(s64)
+ $x0 = COPY %2(s64)
+ $x0 = COPY %2(s64)
+ $x0 = COPY %2(s64)
+)";
+ setUp(MIRString);
+ if (!TM)
+ GTEST_SKIP();
+
+ legacy::PassManager PM;
+ std::unique_ptr<TargetPassConfig> TPC(TM->createPassConfig(PM));
+
+ EraseMockInstructionSelector ISel;
+ ISel.TPC = TPC.get();
+ for (auto &MI : *EntryMBB) {
+ ISel.MIs.push_back(&MI);
+ }
+
+ InstructionSelect ISelPass;
+ ISelPass.setInstructionSelector(&ISel);
+ ISelPass.selectMachineFunction(*MF);
+ EXPECT_EQ(ISel.NumSelected, 3);
+}
+
+} // namespace
More information about the llvm-commits
mailing list