[globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate
Summary:
This allows us to register it with the MachineFunction delegate and be
notified automatically about erasure and creation of instructions. However,
we still need explicit notification for modifications such as those caused
by setReg() or replaceRegWith().
There is a catch with this though. The notification for creation is
delivered before any operands can be added. While appropriate for
scheduling combiner work. This is unfortunate for debug output since an
opcode by itself doesn't provide sufficient information on what happened.
As a result, the work list remembers the instructions (when debug output is
requested) and emits a more complete dump later.
Another nit is that the MachineFunction::Delegate provides const pointers
which is inconvenient since we want to use it to schedule future
modification. To resolve this GISelWorkList now has an optional pointer to
the MachineFunction which describes the scope of the work it is permitted
to schedule. If a given MachineInstr* is in this function then it is
permitted to schedule work to be performed on the MachineInstr's. An
alternative to this would be to remove the const from the
MachineFunction::Delegate interface, however delegates are not permitted
to modify the MachineInstr's they receive.
In addition to this, the observer has three interface changes.
* erasedInstr() is now erasingInstr() to indicate it is about to be erased
but still exists at the moment.
* changingInstr() and changedInstr() have been added to report changes
before and after they are made. This allows us to trace the changes
in the debug output.
* As a convenience changingAllUsesOfReg() and
finishedChangingAllUsesOfReg() will report changingInstr() and
changedInstr() for each use of a given register. This is primarily useful
for changes caused by MachineRegisterInfo::replaceRegWith()
With this in place, both combine rules have been updated to report their
changes to the observer.
Finally, make some cosmetic changes to the debug output and make Combiner
and CombinerHelp
Reviewers: aditya_nandakumar, bogner, volkan, rtereshin, javed.absar
Reviewed By: aditya_nandakumar
Subscribers: mgorny, rovka, kristof.beyls, llvm-commits
Differential Revision: https://reviews.llvm.org/D52947
llvm-svn: 349167
diff --git a/llvm/lib/CodeGen/GlobalISel/CMakeLists.txt b/llvm/lib/CodeGen/GlobalISel/CMakeLists.txt
index 4c1da37..5f13692 100644
--- a/llvm/lib/CodeGen/GlobalISel/CMakeLists.txt
+++ b/llvm/lib/CodeGen/GlobalISel/CMakeLists.txt
@@ -3,6 +3,7 @@
GlobalISel.cpp
Combiner.cpp
CombinerHelper.cpp
+ GISelChangeObserver.cpp
IRTranslator.cpp
InstructionSelect.cpp
InstructionSelector.cpp
diff --git a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
index a2f220a..90fd54e 100644
--- a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
@@ -1,4 +1,4 @@
-//===-- lib/CodeGen/GlobalISel/GICombiner.cpp -----------------------===//
+//===-- lib/CodeGen/GlobalISel/Combiner.cpp -------------------------------===//
//
// The LLVM Compiler Infrastructure
//
@@ -29,36 +29,62 @@
namespace {
/// This class acts as the glue the joins the CombinerHelper to the overall
/// Combine algorithm. The CombinerHelper is intended to report the
-/// modifications it makes to the MIR to the CombinerChangeObserver and the
+/// modifications it makes to the MIR to the GISelChangeObserver and the
/// observer subclass will act on these events. In this case, instruction
/// erasure will cancel any future visits to the erased instruction and
/// instruction creation will schedule that instruction for a future visit.
/// Other Combiner implementations may require more complex behaviour from
-/// their CombinerChangeObserver subclass.
-class WorkListMaintainer : public GISelChangeObserver {
+/// their GISelChangeObserver subclass.
+class WorkListMaintainer : public GISelChangeObserver,
+ public MachineFunction::Delegate {
using WorkListTy = GISelWorkList<512>;
+ MachineFunction &MF;
WorkListTy &WorkList;
+ /// The instructions that have been created but we want to report once they
+ /// have their operands. This is only maintained if debug output is requested.
+ SmallPtrSet<const MachineInstr *, 4> CreatedInstrs;
public:
- WorkListMaintainer(WorkListTy &WorkList) : WorkList(WorkList) {}
- virtual ~WorkListMaintainer() {}
+ WorkListMaintainer(MachineFunction &MF, WorkListTy &WorkList)
+ : GISelChangeObserver(), MF(MF), WorkList(WorkList) {
+ MF.setDelegate(this);
+ }
+ virtual ~WorkListMaintainer() {
+ MF.resetDelegate(this);
+ }
- void erasingInstr(MachineInstr &MI) override {
+ void erasingInstr(const MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << "Erased: " << MI << "\n");
WorkList.remove(&MI);
}
- void createdInstr(MachineInstr &MI) override {
- LLVM_DEBUG(dbgs() << "Created: " << MI << "\n");
+ void createdInstr(const MachineInstr &MI) override {
+ LLVM_DEBUG(dbgs() << "Creating: " << MI << "\n");
+ WorkList.insert(&MI);
+ LLVM_DEBUG(CreatedInstrs.insert(&MI));
+ }
+ void changingInstr(const MachineInstr &MI) override {
+ LLVM_DEBUG(dbgs() << "Changing: " << MI << "\n");
WorkList.insert(&MI);
}
- void changingInstr(MachineInstr &MI) override {
- LLVM_DEBUG(dbgs() << "Changing: " << MI << "\n");
- WorkList.remove(&MI);
- }
- // Currently changed conservatively assumes erased.
- void changedInstr(MachineInstr &MI) override {
+ void changedInstr(const MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << "Changed: " << MI << "\n");
- WorkList.remove(&MI);
+ WorkList.insert(&MI);
+ }
+
+ void reportFullyCreatedInstrs() {
+ LLVM_DEBUG(for (const auto *MI
+ : CreatedInstrs) {
+ dbgs() << "Created: ";
+ MI->print(dbgs());
+ });
+ LLVM_DEBUG(CreatedInstrs.clear());
+ }
+
+ void MF_HandleInsertion(const MachineInstr &MI) override {
+ createdInstr(MI);
+ }
+ void MF_HandleRemoval(const MachineInstr &MI) override {
+ erasingInstr(MI);
}
};
}
@@ -90,8 +116,8 @@
// insert with list bottom up, so while we pop_back_val, we'll traverse top
// down RPOT.
Changed = false;
- GISelWorkList<512> WorkList;
- WorkListMaintainer Observer(WorkList);
+ GISelWorkList<512> WorkList(&MF);
+ WorkListMaintainer Observer(MF, WorkList);
for (MachineBasicBlock *MBB : post_order(&MF)) {
if (MBB->empty())
continue;
@@ -110,8 +136,9 @@
// Main Loop. Process the instructions here.
while (!WorkList.empty()) {
MachineInstr *CurrInst = WorkList.pop_back_val();
- LLVM_DEBUG(dbgs() << "Try combining " << *CurrInst << "\n";);
+ LLVM_DEBUG(dbgs() << "\nTry combining " << *CurrInst;);
Changed |= CInfo.combine(Observer, *CurrInst, Builder);
+ Observer.reportFullyCreatedInstrs();
}
MFChanged |= Changed;
} while (Changed);
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 6018c59..b1c5670 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -1,4 +1,4 @@
-//== ---lib/CodeGen/GlobalISel/GICombinerHelper.cpp --------------------- == //
+//===-- lib/CodeGen/GlobalISel/GICombinerHelper.cpp -----------------------===//
//
// The LLVM Compiler Infrastructure
//
@@ -15,7 +15,7 @@
#include "llvm/CodeGen/MachineRegisterInfo.h"
#include "llvm/CodeGen/TargetInstrInfo.h"
-#define DEBUG_TYPE "gi-combine"
+#define DEBUG_TYPE "gi-combiner"
using namespace llvm;
@@ -23,8 +23,27 @@
MachineIRBuilder &B)
: Builder(B), MRI(Builder.getMF().getRegInfo()), Observer(Observer) {}
-void CombinerHelper::scheduleForVisit(MachineInstr &MI) {
- Observer.createdInstr(MI);
+void CombinerHelper::replaceRegWith(MachineRegisterInfo &MRI, unsigned FromReg,
+ unsigned ToReg) const {
+ Observer.changingAllUsesOfReg(MRI, FromReg);
+
+ if (MRI.constrainRegAttrs(ToReg, FromReg))
+ MRI.replaceRegWith(FromReg, ToReg);
+ else
+ Builder.buildCopy(ToReg, FromReg);
+
+ Observer.finishedChangingAllUsesOfReg();
+}
+
+void CombinerHelper::replaceRegOpWith(MachineRegisterInfo &MRI,
+ MachineOperand &FromRegOp,
+ unsigned ToReg) const {
+ assert(FromRegOp.getParent() && "Expected an operand in an MI");
+ Observer.changingInstr(*FromRegOp.getParent());
+
+ FromRegOp.setReg(ToReg);
+
+ Observer.changedInstr(*FromRegOp.getParent());
}
bool CombinerHelper::tryCombineCopy(MachineInstr &MI) {
@@ -38,7 +57,7 @@
// a(sx) = COPY b(sx) -> Replace all uses of a with b.
if (DstTy.isValid() && SrcTy.isValid() && DstTy == SrcTy) {
MI.eraseFromParent();
- MRI.replaceRegWith(DstReg, SrcReg);
+ replaceRegWith(MRI, DstReg, SrcReg);
return true;
}
return false;
@@ -191,8 +210,11 @@
// type since by definition the result of an extend is larger.
assert(Preferred.Ty != LoadValueTy && "Extending to same type?");
+ LLVM_DEBUG(dbgs() << "Preferred use is: " << *Preferred.MI);
+
// Rewrite the load to the chosen extending load.
unsigned ChosenDstReg = Preferred.MI->getOperand(0).getReg();
+ Observer.changingInstr(MI);
MI.setDesc(
Builder.getTII().get(Preferred.ExtendOpcode == TargetOpcode::G_SEXT
? TargetOpcode::G_SEXTLOAD
@@ -211,7 +233,7 @@
if (UseMI->getOpcode() == Preferred.ExtendOpcode ||
UseMI->getOpcode() == TargetOpcode::G_ANYEXT) {
unsigned UseDstReg = UseMI->getOperand(0).getReg();
- unsigned UseSrcReg = UseMI->getOperand(1).getReg();
+ MachineOperand &UseSrcMO = UseMI->getOperand(1);
const LLT &UseDstTy = MRI.getType(UseDstReg);
if (UseDstReg != ChosenDstReg) {
if (Preferred.Ty == UseDstTy) {
@@ -224,7 +246,7 @@
// rewrites to:
// %2:_(s32) = G_SEXTLOAD ...
// ... = ... %2(s32)
- MRI.replaceRegWith(UseDstReg, ChosenDstReg);
+ replaceRegWith(MRI, UseDstReg, ChosenDstReg);
ScheduleForErase.push_back(UseMO.getParent());
} else if (Preferred.Ty.getSizeInBits() < UseDstTy.getSizeInBits()) {
// If the preferred size is smaller, then keep the extend but extend
@@ -237,7 +259,7 @@
// %2:_(s32) = G_SEXTLOAD ...
// %3:_(s64) = G_ANYEXT %2:_(s32)
// ... = ... %3(s64)
- MRI.replaceRegWith(UseSrcReg, ChosenDstReg);
+ replaceRegOpWith(MRI, UseSrcMO, ChosenDstReg);
} else {
// If the preferred size is large, then insert a truncate. For
// example:
@@ -284,7 +306,9 @@
MachineInstr *PreviouslyEmitted = EmittedInsns.lookup(InsertIntoBB);
if (PreviouslyEmitted) {
+ Observer.changingInstr(*UseMO->getParent());
UseMO->setReg(PreviouslyEmitted->getOperand(0).getReg());
+ Observer.changedInstr(*UseMO->getParent());
continue;
}
@@ -292,14 +316,14 @@
unsigned NewDstReg = MRI.cloneVirtualRegister(MI.getOperand(0).getReg());
MachineInstr *NewMI = Builder.buildTrunc(NewDstReg, ChosenDstReg);
EmittedInsns[InsertIntoBB] = NewMI;
- UseMO->setReg(NewDstReg);
- Observer.createdInstr(*NewMI);
+ replaceRegOpWith(MRI, *UseMO, NewDstReg);
}
for (auto &EraseMI : ScheduleForErase) {
Observer.erasingInstr(*EraseMI);
EraseMI->eraseFromParent();
}
MI.getOperand(0).setReg(ChosenDstReg);
+ Observer.changedInstr(MI);
return true;
}
diff --git a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
new file mode 100644
index 0000000..993a919
--- /dev/null
+++ b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
@@ -0,0 +1,31 @@
+//===-- lib/CodeGen/GlobalISel/GISelChangeObserver.cpp --------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file constains common code to combine machine functions at generic
+// level.
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
+
+using namespace llvm;
+
+void GISelChangeObserver::changingAllUsesOfReg(
+ const MachineRegisterInfo &MRI, unsigned Reg) {
+ for (auto &ChangingMI : MRI.use_instructions(Reg)) {
+ changingInstr(ChangingMI);
+ ChangingAllUsesOfReg.insert(&ChangingMI);
+ }
+}
+
+void GISelChangeObserver::finishedChangingAllUsesOfReg() {
+ for (auto *ChangedMI : ChangingAllUsesOfReg)
+ changedInstr(*ChangedMI);
+}
+
diff --git a/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp b/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
index 74d592b..df2dcac 100644
--- a/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
@@ -81,7 +81,7 @@
LegalizerWorkListManager(InstListTy &Insts, ArtifactListTy &Arts)
: InstList(Insts), ArtifactList(Arts) {}
- void createdInstr(MachineInstr &MI) override {
+ void createdInstr(const MachineInstr &MI) override {
// Only legalize pre-isel generic instructions.
// Legalization process could generate Target specific pseudo
// instructions with generic types. Don't record them
@@ -94,17 +94,17 @@
LLVM_DEBUG(dbgs() << ".. .. New MI: " << MI);
}
- void erasingInstr(MachineInstr &MI) override {
+ void erasingInstr(const MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << ".. .. Erasing: " << MI);
InstList.remove(&MI);
ArtifactList.remove(&MI);
}
- void changingInstr(MachineInstr &MI) override {
+ void changingInstr(const MachineInstr &MI) override {
LLVM_DEBUG(dbgs() << ".. .. Changing MI: " << MI);
}
- void changedInstr(MachineInstr &MI) override {
+ void changedInstr(const MachineInstr &MI) override {
// When insts change, we want to revisit them to legalize them again.
// We'll consider them the same as created.
LLVM_DEBUG(dbgs() << ".. .. Changed MI: " << MI);
@@ -126,8 +126,8 @@
MachineRegisterInfo &MRI = MF.getRegInfo();
// Populate Insts
- InstListTy InstList;
- ArtifactListTy ArtifactList;
+ InstListTy InstList(&MF);
+ ArtifactListTy ArtifactList(&MF);
ReversePostOrderTraversal<MachineFunction *> RPOT(&MF);
// Perform legalization bottom up so we can DCE as we legalize.
// Traverse BB in RPOT and within each basic block, add insts top down,