[MachineLateInstrsCleanup] Handle multiple kills for a preceding definition. #119132
Conversation
|
@llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesWhen removing a redundant definition in order to reuse an earlier identical one it is necessary to remove any earlier kill flag as well. Previously, the assumption has been that any register that kills the defined Reg is enough to handle for this purpose, but this is actually not quite enough. A kill of a super-register does not necessarily imply that all of its subregs (including Reg) is defined at that point: a partial definition of a register is legal. This means Reg may have been killed earlier and is not live at that point. This patch changes the tracking of kill flags to allow for multiple flags to be removed: instead of remembering just the single / latest kill flag, a vector is now used to track and remove them all. TinyPtrVector seems ideal for this as there are only very rarely more than one kill flag, and it doesn't seem to give much difference in compile time. The kill flags handling here is making this pass much more complicated than it would have to be. This pass does not depend on kill flags for its own use, so an interesting alternative to all this handling would be in case there is no real use of them at this late stage, maybe just remove them all. If there actually is a serious user, maybe that pass could instead recompute them and also be able to trust them fully. Also adding an assertion which is unrelated to kill flags, but it seems to make sense (according to liberal assertion policy), to verify that the preceding definition is in fact identical in clearKillsForDef(). This is a little border-line as this requires having an extra argument to that function, but my guess is that this will not matter. Could possibly wait with this as a a separate patch, or even keep skipping it. Fixes #117783 @arsenm @nikic @RKSimon @jayfoad @topperc Full diff: https://github.com/llvm/llvm-project/pull/119132.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp b/llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp
index 6399e8a9523685..5fefa2d8fbbab9 100644
--- a/llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp
+++ b/llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp
@@ -47,9 +47,9 @@ class MachineLateInstrsCleanup : public MachineFunctionPass {
return MI && MI->isIdenticalTo(*ArgMI);
}
};
-
+ typedef SmallDenseMap<Register, TinyPtrVector<MachineInstr *>> Reg2MIVecMap;
std::vector<Reg2MIMap> RegDefs;
- std::vector<Reg2MIMap> RegKills;
+ std::vector<Reg2MIVecMap> RegKills;
// Walk through the instructions in MBB and remove any redundant
// instructions.
@@ -57,7 +57,7 @@ class MachineLateInstrsCleanup : public MachineFunctionPass {
void removeRedundantDef(MachineInstr *MI);
void clearKillsForDef(Register Reg, MachineBasicBlock *MBB,
- BitVector &VisitedPreds);
+ BitVector &VisitedPreds, MachineInstr *ToRemoveMI);
public:
static char ID; // Pass identification, replacement for typeid
@@ -113,19 +113,25 @@ bool MachineLateInstrsCleanup::runOnMachineFunction(MachineFunction &MF) {
// definition.
void MachineLateInstrsCleanup::clearKillsForDef(Register Reg,
MachineBasicBlock *MBB,
- BitVector &VisitedPreds) {
+ BitVector &VisitedPreds,
+ MachineInstr *ToRemoveMI) {
VisitedPreds.set(MBB->getNumber());
- // Kill flag in MBB
- if (MachineInstr *KillMI = RegKills[MBB->getNumber()].lookup(Reg)) {
- KillMI->clearRegisterKills(Reg, TRI);
- return;
- }
+ // Clear kill flag(s) in MBB, that have been seen after the preceding
+ // definition. If Reg or one of its subregs was killed, it would actually
+ // be ok to stop after removing that (and any other) kill-flag, but it
+ // doesn't seem noticeably faster while it would be a bit more complicated.
+ Reg2MIVecMap &MBBKills = RegKills[MBB->getNumber()];
+ if (MBBKills.contains(Reg))
+ for (auto *KillMI : MBBKills[Reg])
+ KillMI->clearRegisterKills(Reg, TRI);
- // Def in MBB (missing kill flag)
- if (MachineInstr *DefMI = RegDefs[MBB->getNumber()].lookup(Reg))
- if (DefMI->getParent() == MBB)
- return;
+ // Definition in current MBB: done.
+ Reg2MIMap &MBBDefs = RegDefs[MBB->getNumber()];
+ MachineInstr *DefMI = MBBDefs[Reg];
+ assert(DefMI->isIdenticalTo(*ToRemoveMI) && "Previous def not identical?");
+ if (DefMI->getParent() == MBB)
+ return;
// If an earlier def is not in MBB, continue in predecessors.
if (!MBB->isLiveIn(Reg))
@@ -133,13 +139,13 @@ void MachineLateInstrsCleanup::clearKillsForDef(Register Reg,
assert(!MBB->pred_empty() && "Predecessor def not found!");
for (MachineBasicBlock *Pred : MBB->predecessors())
if (!VisitedPreds.test(Pred->getNumber()))
- clearKillsForDef(Reg, Pred, VisitedPreds);
+ clearKillsForDef(Reg, Pred, VisitedPreds, ToRemoveMI);
}
void MachineLateInstrsCleanup::removeRedundantDef(MachineInstr *MI) {
Register Reg = MI->getOperand(0).getReg();
BitVector VisitedPreds(MI->getMF()->getNumBlockIDs());
- clearKillsForDef(Reg, MI->getParent(), VisitedPreds);
+ clearKillsForDef(Reg, MI->getParent(), VisitedPreds, MI);
MI->eraseFromParent();
++NumRemoved;
}
@@ -175,7 +181,7 @@ static bool isCandidate(const MachineInstr *MI, Register &DefedReg,
bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
bool Changed = false;
Reg2MIMap &MBBDefs = RegDefs[MBB->getNumber()];
- Reg2MIMap &MBBKills = RegKills[MBB->getNumber()];
+ Reg2MIVecMap &MBBKills = RegKills[MBB->getNumber()];
// Find reusable definitions in the predecessor(s).
if (!MBB->pred_empty() && !MBB->isEHPad() &&
@@ -225,8 +231,8 @@ bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
MBBDefs.erase(Reg);
MBBKills.erase(Reg);
} else if (MI.findRegisterUseOperandIdx(Reg, TRI, true /*isKill*/) != -1)
- // Keep track of register kills.
- MBBKills[Reg] = &MI;
+ // Keep track of all instructions that fully or partially kills Reg.
+ MBBKills[Reg].push_back(&MI);
}
// Record this MI for potential later reuse.
diff --git a/llvm/test/CodeGen/SystemZ/machine-latecleanup-kills.mir b/llvm/test/CodeGen/SystemZ/machine-latecleanup-kills.mir
new file mode 100644
index 00000000000000..51ae602fcd517c
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/machine-latecleanup-kills.mir
@@ -0,0 +1,52 @@
+# RUN: llc -mtriple=s390x-linux-gnu -run-pass=machine-latecleanup %s -o - -mcpu=z16 \
+# RUN: -verify-machineinstrs 2>&1 | FileCheck %s
+
+# Kill flag of $r0q (super-reg) needs to be removed, and $r0l needs to be added as live-in.
+# CHECK-LABEL: name: fun0
+# CHECK-LABEL: bb.0:
+# CHECK: renamable $r0l = LHIMux -1
+# CHECK-NEXT: J %bb.1
+# CHECK-LABEL: bb.1:
+# CHECK-NEXT: liveins: $r0l
+# CHECK: renamable $r1d = LGFI 0
+# CHECK-NEXT: ST128 renamable $r0q, $r15d, 168, $noreg
+# CHECK-NEXT: ST killed renamable $r0l, $r15d, 160, $noreg
+# CHECK-NEXT: Return
+---
+name: fun0
+tracksRegLiveness: true
+body: |
+ bb.0:
+ renamable $r0l = LHIMux -1
+ J %bb.1
+
+ bb.1:
+ renamable $r1d = LGFI 0
+ ST128 killed renamable $r0q, $r15d, 168, $noreg
+ renamable $r0l = LHIMux -1
+ ST killed renamable $r0l, $r15d, 160, $noreg
+ Return
+...
+
+# Kill flags of both $r1d and $r0q (super-reg) need to be removed.
+# CHECK-LABEL: name: fun1
+# CHECK-LABEL: bb.0:
+# CHECK-NEXT: renamable $r1d = LLILL 1
+# CHECK-NEXT: STG renamable $r1d, killed $r15d, 8, $noreg
+# CHECK-NEXT: renamable $r0d = LLILL 0
+# CHECK-NEXT: ST128 renamable $r0q, $r15d, 0, $noreg
+# CHECK-NEXT: STG killed renamable $r1d, killed $r15d, 8, $noreg
+# CHECK-NEXT: Return
+---
+name: fun1
+tracksRegLiveness: true
+body: |
+ bb.0:
+ renamable $r1d = LLILL 1
+ STG killed renamable $r1d, killed $r15d, 8, $noreg
+ renamable $r0d = LLILL 0
+ ST128 killed renamable $r0q, $r15d, 0, $noreg
+ renamable $r1d = LLILL 1
+ STG killed renamable $r1d, killed $r15d, 8, $noreg
+ Return
+...
|
No, there should be no users. All users should be changed to not rely on kill flags |
| if (MBBKills.contains(Reg)) | ||
| for (auto *KillMI : MBBKills[Reg]) | ||
| KillMI->clearRegisterKills(Reg, TRI); |
There was a problem hiding this comment.
Doesn't the [] operator add an entry to the map if its missing? In this case a mapping of Reg to an empty vector.
There was a problem hiding this comment.
Maybe:
if (auto Kills = MBBKills.find(Reg); Kills != MBBKills.end())
for (auto *KillMI : *Kills)
There was a problem hiding this comment.
ok - tried this now, although I think it's a bit less readable.
There seems to be users in the Target backends - which could include late things like expandPostRAPseudo(), perhaps. There are also some use of isKill() in BranchFolding and probably other places. Do you think I could try to just remove kill flags a bit more carelessly ("ahead of time", even in the many cases where there is no following identical definition), and go ahead with this if no tests fail? Or are you saying there is some work left to be done in places that we should aim to do first? |
I tried this now, and found that
It seems there is some assumption there that some register operand must be a kill.
Due to the above and in particular the crash in ARMLoadStoreOpt (which has a lot of checks for kills), I reverted back to the tracking of kill flags and clearing them carefully as per before. I would agree that there is room for improvement here, and at least on SystemZ it seems completely fine to remove any and all kill flags after regalloc. |
|
Ping! Maybe at some point there could be something like a LivenessAnalysis pass that could be required by late passes that actually depend on correct livein-lists and kill flags? That way passes would not be required to preserve this info - which is much more cumbersome than (re)computing it in one place - and users would get all the kill flags including the ones conservatively removed. For now, it seems that MachineLateInstrsCleanup has to continue to update this info as best as it can, with this patch fixing the case of a superregister kill. |
|
Ping! |
|
Ping! Adding some more potential reviewers: |
|
updated per review |
|
Ping! Does this look ok now? |
|
PING! Any objections to this? |
|
Ping. Given that this was a problem reported by the MachineVerifier and that the patch is an improvement which handles cases of super-reg kills, I think I need to push this soon, if no one can see anything wrong with it. The discussion around kill flags remains: I have tried your suggestions to simply remove them liberally, but that hasn't really worked. Tests breaks and even assertions trigger. So I think this is unfortunately a topic that would need a separate discussion: Should we remove the burden on optimizers to update kill flags, and if so how? Find lost optimizations acceptable, or recompute kill flags for passes that really depend on them? |
|
Adding more reviewers with the hopes that someone can take a look at my handling of kill flags here, thanks. @fhahn @MatzeB @jayfoad @topperc @bevin-hansson @serguei-katkov @qcolombet @preames @sdesmalen-arm @perlfu |
| // Definition in current MBB: done. | ||
| Reg2MIMap &MBBDefs = RegDefs[MBB->getNumber()]; | ||
| MachineInstr *DefMI = MBBDefs[Reg]; | ||
| assert(DefMI->isIdenticalTo(*ToRemoveMI) && "Previous def not identical?"); |
There was a problem hiding this comment.
Can you give more detail on the addition of this assert? Specifically because ToRemoveMI seems to exist only to support this?
There was a problem hiding this comment.
Yeah, I guess maybe that assertion could be a separate patch. The whole idea of the pass is to remove a redundant definition because there is an identical dominating one. Given the CFG traversal involved here (DefMI may be in a predecessor), it seems fair to do the assertion. Should I remove it from this patch now, you think?
There was a problem hiding this comment.
I don't disagree with the assertion, just wonder if there had been some scenario that triggered it.
There was a problem hiding this comment.
I think the substance of the change is good. Sub/super register kills definitely need to be handled in this case. This patch seems like a reasonable minimal change to address that need.
Can you add one further test that checks when the defs are super register and a kill is only for a sub register? I don't think existing tests cover this.
| // Definition in current MBB: done. | ||
| Reg2MIMap &MBBDefs = RegDefs[MBB->getNumber()]; | ||
| MachineInstr *DefMI = MBBDefs[Reg]; | ||
| assert(DefMI->isIdenticalTo(*ToRemoveMI) && "Previous def not identical?"); |
There was a problem hiding this comment.
I don't disagree with the assertion, just wonder if there had been some scenario that triggered it.
done. |
|
@perlfu thanks for review! |
…nition. (llvm#119132) When removing a redundant definition in order to reuse an earlier identical one it is necessary to remove any earlier kill flag as well. Previously, the assumption has been that any register that kills the defined Reg is enough to handle for this purpose, but this is actually not quite enough. A kill of a super-register does not necessarily imply that all of its subregs (including Reg) is defined at that point: a partial definition of a register is legal. This means Reg may have been killed earlier and is not live at that point. This patch changes the tracking of kill flags to allow for multiple flags to be removed: instead of remembering just the single / latest kill flag, a vector is now used to track and remove them all. TinyPtrVector seems ideal for this as there are only very rarely more than one kill flag, and it doesn't seem to give much difference in compile time. The kill flags handling here is making this pass much more complicated than it would have to be. This pass does not depend on kill flags for its own use, so an interesting alternative to all this handling would be to just remove them all. If there actually is a serious user, maybe that pass could instead recompute them. Also adding an assertion which is unrelated to kill flags, but it seems to make sense (according to liberal assertion policy), to verify that the preceding definition is in fact identical in clearKillsForDef(). Fixes llvm#117783
When removing a redundant definition in order to reuse an earlier identical one it is necessary to remove any earlier kill flag as well.
Previously, the assumption has been that any register that kills the defined Reg is enough to handle for this purpose, but this is actually not quite enough. A kill of a super-register does not necessarily imply that all of its subregs (including Reg) is defined at that point: a partial definition of a register is legal. This means Reg may have been killed earlier and is not live at that point.
This patch changes the tracking of kill flags to allow for multiple flags to be removed: instead of remembering just the single / latest kill flag, a vector is now used to track and remove them all. TinyPtrVector seems ideal for this as there are only very rarely more than one kill flag, and it doesn't seem to give much difference in compile time.
The kill flags handling here is making this pass much more complicated than it would have to be. This pass does not depend on kill flags for its own use, so an interesting alternative to all this handling would be in case there is no real use of them at this late stage, maybe just remove them all. If there actually is a serious user, maybe that pass could instead recompute them and also be able to trust them fully.
Also adding an assertion which is unrelated to kill flags, but it seems to make sense (according to liberal assertion policy), to verify that the preceding definition is in fact identical in clearKillsForDef(). This is a little border-line as this requires having an extra argument to that function, but my guess is that this will not matter. Could possibly wait with this as a a separate patch, or even keep skipping it.
Fixes #117783
@arsenm @nikic @RKSimon @jayfoad @topperc @uweigand