[PowerPC] Fix the killed flag in mi-peephole pass

[qiucf]

Summary

When doing the peephole, we move the killed flag from srcMI to destMI without do more check,
this is not right.

For below example, %3 is in a loop, %2 is out of the loop.

bb1:
   %2:g8rc = RLDICL killed %1, 0, 32
bb2:
   %3:g8rc = RLDICR %2, 2, 61    // can fold to %3:g8rc = RLDIC %1, 2, 30, %1 shouldn't has the kill flag.
   ...
   BCC 4, $cr0, %bb.2

This patch uses the conservative method to set the kill flag.
It can fix 62 lnt erros and 22 lit errors.

[ZhangKang]

Fix the error case.

[shchenz]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1580

Should we also consider SrcMI->getOperand(0) or legacy MI.getOperand(1) at least when MI and and SrcMI are in same BB?

We have a API fixupIsDeadOrKill for kill/dead flag fixup.

[ZhangKang]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1580

The SrcMI will be removed if it's necessary in the line 1571.

The fixupIsDeadOrkill is used for non-ssa, but the pass is a SSA pass. So, we can't using it.

[shchenz]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1580

The function fixupIsDeadOrkill should be extended to non-ssa in patch https://reviews.llvm.org/D81723.

If you want to completely fix up the kill/dead flag, there are some situations need to be considered, like:
1:

%2:g8rc = RLDICL %1, 0, 32
STORE killed %1
%3:g8rc = RLDICR %2, 2, 61   ;; killed flag from above STORE should be sunk down here

2:

 %2:g8rc = RLDICL killed %1, 0, 32
STORE  %2
 %3:g8rc = RLDICR killed %2, 2, 61 ;;  killed flag for %2 should be hoisted up to above STORE

fixupIsDeadOrkill can do this for you.

[ZhangKang]

Call IsKilledFor() to fix the flag.

[ZhangKang]

Remove the dump code.

[shchenz]

Please also add test case for the new scenarios

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1597

I think you mean SrcMI->getOperand(1)?

[ZhangKang]

Add a new func setKillAfterFolding.

[nemanjai]

It seems like we keep adding more and more complexity here to handle these flags. Is there not a pass we can just run that will simply re-compute this stuff (at least for the use of this while we're in SSA)? What does LiveVariables do? We certainly end up running DCE after the MI peephole to get rid of dead instructions after the transformation. It seems like we should be able to do the same thing for killed/dead flags as well.

[qiucf]

Since the motivation case is fixed by rG69289cc1 and this patch is a little bit aggressive (causing more crashes), close it.

llvm/lib/Target/PowerPC/PPCInstrInfo.h

public:
   /// 3. We can do accurate fixup for the case when all instructions between
   ///    [\p StartMI, \p EndMI] are in same basic block.
   /// 4. For the case when \p StartMI and \p EndMI are not in same basic block,
   ///    we conservatively clear kill flag for all uses of \p RegNo for pre-RA
   ///    and for post-RA, we give an assertion as without reaching definition
   ///    analysis post-RA, \p StartMI and \p EndMI are hard to keep right.
   void fixupIsDeadOrKill(MachineInstr &StartMI, MachineInstr &EndMI,
                          unsigned RegNo) const;
+
+  /// Set the killed/dead flag when folding \p DefMI and \p MI for SSA.
+  /// \p ForwardingMO is the forwarding operand used in MI, \p ForwardedMO is
+  /// the forwarded operand used in \p DefMI, its Reg is used by MI after the
+  /// folding. Before the folding, \p ForwardingMOHasOneUse is show that whether
+  /// the forwarding operand has only one use, \p ForwardingKilledOperandReg is
+  /// show that whether the forwarding operand has the killed flag.
+  void setKillAfterFolding(MachineInstr &MI, MachineInstr &DefMI,
+                           MachineOperand &ForwardingMO,
+                           MachineOperand &ForwardedMO,
+                           bool ForwardingMOHasOneUse,
+                           unsigned ForwardingKilledOperandReg = ~0U) const;
+
   void replaceInstrWithLI(MachineInstr &MI, const LoadImmediateInfo &LII) const;
   void replaceInstrOperandWithImm(MachineInstr &MI, unsigned OpNo,
                                   int64_t Imm) const;
 
   bool instrHasImmForm(unsigned Opc, bool IsVFReg, ImmInstrInfo &III,
                        bool PostRA) const;
 
   // In PostRA phase, try to find instruction defines \p Reg before \p MI.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

for (; It != E; ++It) {
     if ((&*It) == &StartMI)
       break;
   }
   // Ensure RegMo liveness is killed after EndMI.
   assert((IsKillSet || (MO && MO->isDead())) &&
          "RegNo should be killed or dead");
 }
 
+void PPCInstrInfo::setKillAfterFolding(
+    MachineInstr &MI, MachineInstr &DefMI,
+    MachineOperand &ForwardingMO, MachineOperand &ForwardedMO,
+    bool ForwardingMOHasOneUse,
+    unsigned ForwardingKilledOperandReg) const {
+  MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+  assert(MRI.isSSA() && "Only used for SSA form.");
+
+  if (!ForwardingMOHasOneUse && !ForwardingMO.isKill()) {
+    // FIXME: Set the kill/dead flag if ForwardingMO has more than one use and
+    // not the last use before the folding. Below is an example:
+    //
+    // bb1:
+    //   %2:g8rc = RLDICL killed %1, 0, 32
+    //   %3:g8rc = RLDICR %2, 2, 61
+    // bb2:
+    //   %4:g8rc = RLDICR %2, 2, 61
+    ForwardedMO.setIsKill(false);
+  } else if (DefMI.getParent() == MI.getParent()) {
+    // Check if reg is killed between MI and DefMI.
+    auto IsKilledFor = [&](unsigned Reg) {
+      MachineBasicBlock::const_reverse_iterator It = MI;
+      MachineBasicBlock::const_reverse_iterator E = DefMI;
+      It++;
+      for (; It != E; ++It) {
+        if (It->killsRegister(Reg))
+          return true;
+      }
+      return false;
+    };
+
+    // Update kill flag
+    if (ForwardedMO.isKill() || IsKilledFor(ForwardedMO.getReg())) {
+      // For below example, we call fixupIsDeadOrKill() to move the killed %1
+      // from STORE to RLDIC after folding.
+      //
+      // %2:g8rc = RLDICL %1, 0, 32           | %2:g8rc = RLDICL %1, 0, 32
+      // STORE killed %1                  ==> | STORE %1
+      // %3:g8rc = RLDICR %2, 2, 61           | %3:g8rc = RLDIC killed %1, 2, 30
+      fixupIsDeadOrKill(DefMI, MI, ForwardedMO.getReg());
+    } if (ForwardingKilledOperandReg != ~0U) {
+      // For below example, we call fixupIsDeadOrKill() to move the killed %2
+      // from RLDICR to STORE and killed %1 from RLDICL to RLDIC after folding.
+      //
+      // %2:g8rc = RLDICL killed %1, 0, 32    | %2:g8rc = RLDICL %1, 0, 32
+      // STORE %2                         ==> | STORE killed %2
+      // %3:g8rc = RLDICR killed %2, 2, 61    | %3:g8rc = RLDIC killed %1, 2, 30
+      fixupIsDeadOrKill(DefMI, MI, ForwardingKilledOperandReg);
+    }
+  } else if (DefMI.getOperand(1).isKill()) {
+    // FIXME: If MI and DefMI are not in same basic block. we should based on
+    // the CFG to set kill/dead flag. For below example, %3 is in a loop, but
+    // %2 is out of the loop.
+    //
+    // bb1:
+    //   %2:g8rc = RLDICL killed %1, 0, 32
+    // bb2:
+    //   %3:g8rc = RLDICR %2, 2, 61
+    //   ...
+    //   BCC 4, $cr0, %bb.2
+    DefMI.getOperand(1).setIsKill(false);
+  }
+}
+
 // This opt tries to convert the following imm form to an index form to save an
 // add for stack variables.
 // Return false if no such pattern found.
 //
 // ADDI instr: ToBeChangedReg = ADDI FrameBaseReg, OffsetAddi
 // ADD instr:  ToBeDeletedReg = ADD ToBeChangedReg(killed), ScaleReg
 // Imm instr:  Reg            = op OffsetImm, ToBeDeletedReg(killed)
 //
if (MI.getOperand(III.OpNoForForwarding).isKill())
     ForwardKilledOperandReg = MI.getOperand(III.OpNoForForwarding).getReg();
 
   // Do the transform
   LLVM_DEBUG(dbgs() << "Replacing instruction:\n");
   LLVM_DEBUG(MI.dump());
   LLVM_DEBUG(dbgs() << "Fed by:\n");
   LLVM_DEBUG(DefMI.dump());
 
+  bool HasOneUse =
+      MRI->hasOneUse(MI.getOperand(III.OpNoForForwarding).getReg());
+
+  // The default kill flag for MI.getOperand(III.OpNoForForwarding) is false.
   MI.getOperand(III.OpNoForForwarding).setReg(RegMO->getReg());
-  MI.getOperand(III.OpNoForForwarding).setIsKill(RegMO->isKill());
   MI.getOperand(III.ImmOpNo).setImm(Imm);
 
-  // FIXME: fix kill/dead flag if MI and DefMI are not in same basic block.
-  if (DefMI.getParent() == MI.getParent()) {
-    // Check if reg is killed between MI and DefMI.
-    auto IsKilledFor = [&](unsigned Reg) {
-      MachineBasicBlock::const_reverse_iterator It = MI;
-      MachineBasicBlock::const_reverse_iterator E = DefMI;
-      It++;
-      for (; It != E; ++It) {
-        if (It->killsRegister(Reg))
-          return true;
-      }
-      return false;
-    };
-
-    // Update kill flag
-    if (RegMO->isKill() || IsKilledFor(RegMO->getReg()))
-      fixupIsDeadOrKill(DefMI, MI, RegMO->getReg());
-    if (ForwardKilledOperandReg != ~0U)
-      fixupIsDeadOrKill(DefMI, MI, ForwardKilledOperandReg);
-  }
+  setKillAfterFolding(MI, DefMI, MI.getOperand(III.OpNoForForwarding), *RegMO,
+                      HasOneUse, ForwardKilledOperandReg);
 
   LLVM_DEBUG(dbgs() << "With:\n");
   LLVM_DEBUG(MI.dump());
   return true;
 }
 
 // If an X-Form instruction is fed by an add-immediate and one of its operands
 // is the literal zero, attempt to forward the source of the add-immediate to

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

for (MachineInstr &MI : MBB) {
             MI.getOperand(1).ChangeToImmediate(0);
             MI.setDesc(TII->get(Is64Bit ? PPC::LI8 : PPC::LI));
           } else {
             // Replace MI with "ANDI_rec reg, 0"
             MI.RemoveOperand(4);
             MI.RemoveOperand(3);
             MI.getOperand(2).setImm(0);
             MI.setDesc(TII->get(Is64Bit ? PPC::ANDI8_rec : PPC::ANDI_rec));
+
+            // Get killed info in case fixup needed after transformation.
+            unsigned ForwardKilledOperandReg = ~0U;
+            if (MI.getOperand(1).isKill())
+               ForwardKilledOperandReg = MI.getOperand(1).getReg();
+            bool HasOneUse = MRI->hasOneUse(MI.getOperand(1).getReg());
+
             MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
-            if (SrcMI->getOperand(1).isKill()) {
-              MI.getOperand(1).setIsKill(true);
-              SrcMI->getOperand(1).setIsKill(false);
-            } else
-              // About to replace MI.getOperand(1), clear its kill flag.
-              MI.getOperand(1).setIsKill(false);
+            TII->setKillAfterFolding(MI, *SrcMI, MI.getOperand(1),
+                                     SrcMI->getOperand(1), HasOneUse,
+                                     ForwardKilledOperandReg);
           }
 
           LLVM_DEBUG(dbgs() << "With: ");
           LLVM_DEBUG(MI.dump());
         } else if ((isRunOfOnes((unsigned)(FinalMask.getZExtValue()), NewMB,
                                NewME) && NewMB <= NewME)|| SrcMaskFull) {
           // Here we only handle MBMI <= MEMI case, so NewMB must be no bigger
           // than NewME. Otherwise we get a 64 bit value after folding, but MI
           // return a 32 bit value.
 
           Simplified = true;
           LLVM_DEBUG(dbgs() << "Converting Instr: ");
           LLVM_DEBUG(MI.dump());
 
           uint16_t NewSH = (SHSrc + SHMI) % 32;
           MI.getOperand(2).setImm(NewSH);
           // If SrcMI mask is full, no need to update MBMI and MEMI.
           if (!SrcMaskFull) {
             MI.getOperand(3).setImm(NewMB);
             MI.getOperand(4).setImm(NewME);
           }
+
+          // Get killed info in case fixup needed after transformation.
+          unsigned ForwardKilledOperandReg = ~0U;
+          if (MI.getOperand(1).isKill())
+             ForwardKilledOperandReg = MI.getOperand(1).getReg();
+          bool HasOneUse = MRI->hasOneUse(MI.getOperand(1).getReg());
+
           MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
-          if (SrcMI->getOperand(1).isKill()) {
-            MI.getOperand(1).setIsKill(true);
-            SrcMI->getOperand(1).setIsKill(false);
-          } else
-            // About to replace MI.getOperand(1), clear its kill flag.
-            MI.getOperand(1).setIsKill(false);
+          TII->setKillAfterFolding(MI, *SrcMI, MI.getOperand(1),
+                                   SrcMI->getOperand(1), HasOneUse,
+                                   ForwardKilledOperandReg);
 
           LLVM_DEBUG(dbgs() << "To: ");
           LLVM_DEBUG(MI.dump());
         }
         if (Simplified) {
           // If FoldingReg has no non-debug use and it has no implicit def (it
           // is not RLWINMO or RLWINM8o), it's safe to delete its def SrcMI.
           // Otherwise keep it.
bool PPCMIPeephole::emitRLDICWhenLoweringJumpTables(MachineInstr &MI) {
   // The bits cleared with RLDICR are (MEMI, 63].
   // After the sequence, the bits cleared are:
   // [0, MBSrc-SHMI) and (MEMI, 63).
   //
   // The bits cleared with RLDIC are [0, NewMB) and (63-NewSH, 63].
   if ((63 - NewSH) != MEMI)
     return false;
 
+  // Get killed info in case fixup needed after transformation.
+  unsigned ForwardKilledOperandReg = ~0U;
+  if (MI.getOperand(1).isKill())
+    ForwardKilledOperandReg = MI.getOperand(1).getReg();
+
+
   LLVM_DEBUG(dbgs() << "Converting pair: ");
   LLVM_DEBUG(SrcMI->dump());
   LLVM_DEBUG(MI.dump());
 
+  bool HasOneUse = MRI->hasOneUse(MI.getOperand(1).getReg());
+
   MI.setDesc(TII->get(PPC::RLDIC));
+
+  // The default kill flag for MI.getOperand(1) is false.
   MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
   MI.getOperand(2).setImm(NewSH);
   MI.getOperand(3).setImm(NewMB);
 
+  TII->setKillAfterFolding(MI, *SrcMI, MI.getOperand(1), SrcMI->getOperand(1),
+                           HasOneUse, ForwardKilledOperandReg);
+
   LLVM_DEBUG(dbgs() << "To: ");
   LLVM_DEBUG(MI.dump());
   NumRotatesCollapsed++;
   // If SrcReg has no non-debug use it's safe to delete its def SrcMI.
   if (MRI->use_nodbg_empty(SrcReg)) {
     assert(!SrcMI->hasImplicitDef() &&
            "Not expecting an implicit def with this instr.");
     SrcMI->eraseFromParent();
   }
   return true;
 }
 
 // For case in LLVM IR
 // entry:
 //   %iconv = sext i32 %index to i64
 //   br i1 undef label %true, label %false
 // true:
 //   %ptr = getelementptr inbounds i32, i32* null, i64 %iconv
 // ...
 // PPCISelLowering::combineSHL fails to combine, because sext and shl are in
 // different BBs when conducting instruction selection. We can do a peephole
 // optimization to combine these two instructions into extswsli after
 // instruction selection.
 bool PPCMIPeephole::combineSEXTAndSHL(MachineInstr &MI,
                                       MachineInstr *&ToErase) {
   if (MI.getOpcode() != PPC::RLDICR)

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

 ...
 # CHECK-LABEL: test:
 # CHECK: # %bb.0: # %entry
 # CHECK-NEXT:   rldic 3, 4, 2, 30
 # CHECK-NEXT:   blr
 #
 # CHECK-PASS-NOT:     %2:g8rc = RLDICL killed %1, 0, 32
 # CHECK-PASS-NOT:     %3:g8rc = RLDICR %2, 2, 61
-# CHECK-PASS:     %3:g8rc = RLDIC %1, 2, 30
+# CHECK-PASS:     %3:g8rc = RLDIC killed %1, 2, 30

llvm/test/CodeGen/PowerPC/mi-peephole-kill.mir

body:             |
   ; CHECK: bb.0.entry:
   ; successors: %bb.1(0x50000000), %bb.3(0x30000000)
   ; CHECK:   liveins: $x3
   ; CHECK:   %2:g8rc = COPY $x3
   ; CHECK:   %3:g8rc = RLDICR %2, 2, 61
   ; CHECK:   %4:g8rc_and_g8rc_nox0 = ADDIStocHA8 $x2, @global
   ; CHECK:   %5:g8rc = ADDItocL killed %4, @global
   ; CHECK:   %6:g8rc_and_g8rc_nox0 = ADD8 killed %5, killed %3
-  ; CHECK:   %0:g8rc_and_g8rc_nox0 = ADDI8 killed %6, 40
+  ; CHECK:   %0:g8rc_and_g8rc_nox0 = ADDI8 %6, 40
   ; CHECK:   %7:g8rc_and_g8rc_nox0 = RLDICL %0, 61, 3
   ; CHECK:   %8:g8rc = LI8 1
   ; CHECK:   %9:g8rc = RLDICR killed %8, 44, 19
   ; CHECK:   %10:gprc = LBZX killed %7, killed %9 :: (load 1 from %ir.6)
   ; CHECK:   %11:crrc = CMPLWI %10, 0
   ; CHECK:   BCC 76, killed %11, %bb.3
   ; CHECK:   B %bb.1
 
body:             |
   ; CHECK: bb.2 (%ir-block.16):
   ; CHECK:   successors:
   ; CHECK:   ADJCALLSTACKDOWN 32, 0, implicit-def dead $r1, implicit $r1
   ; CHECK:   $x3 = COPY %0
   ; CHECK:   BL8_NOP @__asan_report_load4, csr_ppc64_altivec, implicit-def dead $lr8, implicit $rm, implicit $x3, implicit-def $r1
   ; CHECK:   ADJCALLSTACKUP 32, 0, implicit-def dead $r1, implicit $r1
 
   ; CHECK: bb.3 (%ir-block.19):
-  ; CHECK:   %16:gprc_and_gprc_nor0 = LWZ 40, killed %6 :: (load 4 from %ir.20)
+  ; CHECK:   %16:gprc_and_gprc_nor0 = LWZ 40, %6 :: (load 4 from %ir.20)
   ; CHECK:   %17:gprc = nsw ADDI killed %16, 1
-  ; CHECK:   STW killed %17, 40, killed %6 :: (store 4 into %ir.20)
+  ; CHECK:   STW killed %17, 40, %6 :: (store 4 into %ir.20)
   ; CHECK:   BLR8 implicit $lr8, implicit $rm
 ...

llvm/test/CodeGen/PowerPC/mi-peephole.mir

bb.0.entry:
     %3:g8rc = RLDICR %2, 2, 61
     $x3 = COPY %3
     BLR8 implicit $lr8, implicit $rm, implicit $x3
 
   ; CHECK-LABEL: testRLDIC
   ; CHECK: bb.0.entry:
   ; CHECK:   %1:g8rc = COPY $x4
   ; CHECK:   %0:g8rc = COPY $x3
-  ; CHECK:   %3:g8rc = RLDIC %1, 2, 30
+  ; CHECK:   %3:g8rc = RLDIC killed %1, 2, 30
   ; CHECK:   $x3 = COPY %3
   ; CHECK:   BLR8 implicit $lr8, implicit $rm, implicit $x3
 ...