Skip to content

Commit 91dfdd5

Browse files
committed
[globalisel] Rename GISelChangeObserver's erasedInstr() to erasingInstr() and related nits. NFC
Summary: There's little of interest that can be done to an already-erased instruction. You can't inspect it, write it to a debug log, etc. It ought to be notification that we're about to erase it. Rename the function to clarify the timing of the event and reflect current usage. Also fixed one case where we were trying to print an erased instruction. Reviewers: aditya_nandakumar Reviewed By: aditya_nandakumar Subscribers: rovka, kristof.beyls, llvm-commits Differential Revision: https://reviews.llvm.org/D55611 llvm-svn: 348976
1 parent d1c6186 commit 91dfdd5

File tree

6 files changed

+11
-14
lines changed

6 files changed

+11
-14
lines changed

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ class CombinerHelper {
3030
MachineRegisterInfo &MRI;
3131
GISelChangeObserver &Observer;
3232

33-
void eraseInstr(MachineInstr &MI);
3433
void scheduleForVisit(MachineInstr &MI);
3534

3635
public:

llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,16 @@
1818
namespace llvm {
1919
/// Abstract class that contains various methods for clients to notify about
2020
/// changes. This should be the preferred way for APIs to notify changes.
21-
/// Typically calling erasedInstr/createdInstr multiple times should not affect
21+
/// Typically calling erasingInstr/createdInstr multiple times should not affect
2222
/// the result. The observer would likely need to check if it was already
2323
/// notified earlier (consider using GISelWorkList).
2424
class MachineInstr;
2525
class GISelChangeObserver {
2626
public:
2727
virtual ~GISelChangeObserver() {}
2828

29-
/// An instruction was erased.
30-
virtual void erasedInstr(MachineInstr &MI) = 0;
29+
/// An instruction is about to be erased.
30+
virtual void erasingInstr(MachineInstr &MI) = 0;
3131
/// An instruction was created and inserted into the function.
3232
virtual void createdInstr(MachineInstr &MI) = 0;
3333
/// This instruction was mutated in some way.

llvm/lib/CodeGen/GlobalISel/Combiner.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ class WorkListMaintainer : public GISelChangeObserver {
4343
WorkListMaintainer(WorkListTy &WorkList) : WorkList(WorkList) {}
4444
virtual ~WorkListMaintainer() {}
4545

46-
void erasedInstr(MachineInstr &MI) override {
46+
void erasingInstr(MachineInstr &MI) override {
4747
LLVM_DEBUG(dbgs() << "Erased: "; MI.print(dbgs()); dbgs() << "\n");
4848
WorkList.remove(&MI);
4949
}

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,6 @@ CombinerHelper::CombinerHelper(GISelChangeObserver &Observer,
2323
MachineIRBuilder &B)
2424
: Builder(B), MRI(Builder.getMF().getRegInfo()), Observer(Observer) {}
2525

26-
void CombinerHelper::eraseInstr(MachineInstr &MI) {
27-
Observer.erasedInstr(MI);
28-
}
2926
void CombinerHelper::scheduleForVisit(MachineInstr &MI) {
3027
Observer.createdInstr(MI);
3128
}
@@ -299,8 +296,8 @@ bool CombinerHelper::tryCombineExtendingLoads(MachineInstr &MI) {
299296
Observer.createdInstr(*NewMI);
300297
}
301298
for (auto &EraseMI : ScheduleForErase) {
299+
Observer.erasingInstr(*EraseMI);
302300
EraseMI->eraseFromParent();
303-
Observer.erasedInstr(*EraseMI);
304301
}
305302
MI.getOperand(0).setReg(ChosenDstReg);
306303

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ class LegalizerWorkListManager : public GISelChangeObserver {
9494
LLVM_DEBUG(dbgs() << ".. .. New MI: " << MI;);
9595
}
9696

97-
void erasedInstr(MachineInstr &MI) override {
97+
void erasingInstr(MachineInstr &MI) override {
98+
LLVM_DEBUG(dbgs() << ".. .. Erasing: " << MI;);
9899
InstList.remove(&MI);
99100
ArtifactList.remove(&MI);
100101
}
@@ -146,7 +147,7 @@ bool Legalizer::runOnMachineFunction(MachineFunction &MF) {
146147
const LegalizerInfo &LInfo(Helper.getLegalizerInfo());
147148
LegalizationArtifactCombiner ArtCombiner(Helper.MIRBuilder, MF.getRegInfo(), LInfo);
148149
auto RemoveDeadInstFromLists = [&WorkListObserver](MachineInstr *DeadMI) {
149-
WorkListObserver.erasedInstr(*DeadMI);
150+
WorkListObserver.erasingInstr(*DeadMI);
150151
};
151152
bool Changed = false;
152153
do {
@@ -175,15 +176,15 @@ bool Legalizer::runOnMachineFunction(MachineFunction &MF) {
175176
MachineInstr &MI = *ArtifactList.pop_back_val();
176177
assert(isPreISelGenericOpcode(MI.getOpcode()) && "Expecting generic opcode");
177178
if (isTriviallyDead(MI, MRI)) {
178-
LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
179+
LLVM_DEBUG(dbgs() << MI << "Is dead\n");
179180
RemoveDeadInstFromLists(&MI);
180181
MI.eraseFromParentAndMarkDBGValuesForRemoval();
181182
continue;
182183
}
183184
SmallVector<MachineInstr *, 4> DeadInstructions;
184185
if (ArtCombiner.tryCombineInstruction(MI, DeadInstructions)) {
185186
for (auto *DeadMI : DeadInstructions) {
186-
LLVM_DEBUG(dbgs() << ".. Erasing Dead Instruction " << *DeadMI);
187+
LLVM_DEBUG(dbgs() << *DeadMI << "Is dead\n");
187188
RemoveDeadInstFromLists(DeadMI);
188189
DeadMI->eraseFromParentAndMarkDBGValuesForRemoval();
189190
}

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ class DummyGISelObserver : public GISelChangeObserver {
1515
public:
1616
void changedInstr(MachineInstr &MI) override {}
1717
void createdInstr(MachineInstr &MI) override {}
18-
void erasedInstr(MachineInstr &MI) override {}
18+
void erasingInstr(MachineInstr &MI) override {}
1919
};
2020

2121
// Test CTTZ expansion when CTTZ_ZERO_UNDEF is legal or custom,

0 commit comments

Comments
 (0)