Updates per review.

Author: JonPsson1

llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp

@@ -40,25 +40,24 @@ namespace {
 class MachineLateInstrsCleanup {
   const TargetRegisterInfo *TRI = nullptr;
   const TargetInstrInfo *TII = nullptr;
+  const MachineRegisterInfo *MRI = nullptr;
 
-  // Data structures to map regs to their definitions and kills per MBB.
+  // Data structure to map regs to their definitions per MBB.
   struct Reg2MIMap : public SmallDenseMap<Register, MachineInstr *> {
     bool hasIdentical(Register Reg, MachineInstr *ArgMI) {
       MachineInstr *MI = lookup(Reg);
       return MI && MI->isIdenticalTo(*ArgMI);
     }
   };
-  typedef SmallDenseMap<Register, TinyPtrVector<MachineInstr *>> Reg2MIVecMap;
   std::vector<Reg2MIMap> RegDefs;
-  std::vector<Reg2MIVecMap> RegKills;
 
   // Walk through the instructions in MBB and remove any redundant
   // instructions.
   bool processBlock(MachineBasicBlock *MBB);
 
   void removeRedundantDef(MachineInstr *MI);
-  void clearKillsForDef(Register Reg, MachineBasicBlock *MBB,
-                        BitVector &VisitedPreds, MachineInstr *ToRemoveMI);
+  void updateLiveInLists(Register Reg, MachineBasicBlock *MBB,
+                         BitVector &VisitedPreds, MachineInstr *ToRemoveMI);
 
 public:
   bool run(MachineFunction &MF);
@@ -116,11 +115,10 @@ MachineLateInstrsCleanupPass::run(MachineFunction &MF,
 bool MachineLateInstrsCleanup::run(MachineFunction &MF) {
   TRI = MF.getSubtarget().getRegisterInfo();
   TII = MF.getSubtarget().getInstrInfo();
+  MRI = &MF.getRegInfo();
 
   RegDefs.clear();
   RegDefs.resize(MF.getNumBlockIDs());
-  RegKills.clear();
-  RegKills.resize(MF.getNumBlockIDs());
 
   // Visit all MBBs in an order that maximises the reuse from predecessors.
   bool Changed = false;
@@ -133,41 +131,36 @@ bool MachineLateInstrsCleanup::run(MachineFunction &MF) {
 
 // Clear any preceding kill flag on Reg after removing a redundant
 // definition.
-void MachineLateInstrsCleanup::clearKillsForDef(Register Reg,
+void MachineLateInstrsCleanup::updateLiveInLists(Register Reg,
                                                 MachineBasicBlock *MBB,
                                                 BitVector &VisitedPreds,
                                                 MachineInstr *ToRemoveMI) {
   VisitedPreds.set(MBB->getNumber());
 
-  // 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);
-
   // 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 the earlier def is not in MBB, it has now become live in. Continue in
+  // predecessors until the defining MBB has been reached.
   if (!MBB->isLiveIn(Reg))
     MBB->addLiveIn(Reg);
   assert(!MBB->pred_empty() && "Predecessor def not found!");
   for (MachineBasicBlock *Pred : MBB->predecessors())
     if (!VisitedPreds.test(Pred->getNumber()))
-      clearKillsForDef(Reg, Pred, VisitedPreds, ToRemoveMI);
+      updateLiveInLists(Reg, Pred, VisitedPreds, ToRemoveMI);
 }
 
 void MachineLateInstrsCleanup::removeRedundantDef(MachineInstr *MI) {
   Register Reg = MI->getOperand(0).getReg();
+  // Clear any and all kill flags.
+  for (MCPhysReg SReg : TRI->superregs_inclusive(Reg))
+    MRI->clearKillFlags(SReg);
   BitVector VisitedPreds(MI->getMF()->getNumBlockIDs());
-  clearKillsForDef(Reg, MI->getParent(), VisitedPreds, MI);
+  updateLiveInLists(Reg, MI->getParent(), VisitedPreds, MI);
   MI->eraseFromParent();
   ++NumRemoved;
 }
@@ -203,7 +196,6 @@ static bool isCandidate(const MachineInstr *MI, Register &DefedReg,
 bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
   bool Changed = false;
   Reg2MIMap &MBBDefs = RegDefs[MBB->getNumber()];
-  Reg2MIVecMap &MBBKills = RegKills[MBB->getNumber()];
 
   // Find reusable definitions in the predecessor(s).
   if (!MBB->pred_empty() && !MBB->isEHPad() &&
@@ -230,7 +222,6 @@ bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
     // it) are valid.
     if (MI.modifiesRegister(FrameReg, TRI)) {
       MBBDefs.clear();
-      MBBKills.clear();
       continue;
     }
 
@@ -249,20 +240,15 @@ bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
     // Clear any entries in map that MI clobbers.
     for (auto DefI : llvm::make_early_inc_range(MBBDefs)) {
       Register Reg = DefI.first;
-      if (MI.modifiesRegister(Reg, TRI)) {
+      if (MI.modifiesRegister(Reg, TRI))
         MBBDefs.erase(Reg);
-        MBBKills.erase(Reg);
-      } else if (MI.findRegisterUseOperandIdx(Reg, TRI, true /*isKill*/) != -1)
-        // Keep track of all instructions that fully or partially kills Reg.
-        MBBKills[Reg].push_back(&MI);
     }
 
     // Record this MI for potential later reuse.
     if (IsCandidate) {
       LLVM_DEBUG(dbgs() << "Found interesting instruction in "
                         << printMBBReference(*MBB) << ":  " << MI);
       MBBDefs[DefedReg] = &MI;
-      assert(!MBBKills.count(DefedReg) && "Should already have been removed.");
     }
   }
 

llvm/test/CodeGen/SystemZ/machine-latecleanup-kills.mir

@@ -1,21 +1,26 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
 # 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:             |
+  ; CHECK-LABEL: name: fun0
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   renamable $r0l = LHIMux -1
+  ; CHECK-NEXT:   J %bb.1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   liveins: $r0l
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   renamable $r1d = LGFI 0
+  ; CHECK-NEXT:   ST128 renamable $r0q, $r15d, 168, $noreg
+  ; CHECK-NEXT:   ST renamable $r0l, $r15d, 160, $noreg
+  ; CHECK-NEXT:   Return
   bb.0:
     renamable $r0l = LHIMux -1
     J %bb.1
@@ -29,19 +34,18 @@ body:             |
 ...
 
 # 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:
+    ; CHECK-LABEL: name: fun1
+    ; CHECK: 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 renamable $r1d, killed $r15d, 8, $noreg
+    ; CHECK-NEXT: Return
     renamable $r1d = LLILL 1
     STG killed renamable $r1d, killed $r15d, 8, $noreg
     renamable $r0d = LLILL 0