[PowerPC] in fixupIsDeadOrKill before RA, get def instructions by calling getVRegDef

[shchenz]

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=47041

%1:gprc_and_gprc_nor0 = LI 0        ------>DefMI
%2:gprc = COPY %1:gprc_and_gprc_nor0
%3:gprc = ORI killed %2:gprc, 1      ------>MI

%3:gprc can be simplified to LI 1, so killed flag for %2:gprc should be hoisted up to other use or to def.
Because we set StartMI as DefMI which is through COPY like instructions before RA, so StartMI is %1:gprc_and_gprc_nor0 instead of %2:gprc, then when we fixup killed/dead flag for %2:gprc, we get %2:gprc redefine asserts.

This patch fixes this assertion, before RA, get real definition by calling getVRegDef instead of using StartMI.

[dim]

For me, this fixes the assertion for both the original test case (a fully preprocessed ASTReader.cpp) and the minimized test case from https://bugs.llvm.org/show_bug.cgi?id=47041#c1

[shchenz]

make sure StartMI is a def through COPY

[shchenz]

gentle ping...

[lkail]

LGTM with nits.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

2676

nit: findRegisterUseOperandIdx(RegNo) < 0 -> readsRegister(RegNo)

llvm/lib/Target/PowerPC/PPCInstrInfo.h

579

Could you add more comment about the fact StartMI might be altered?

[lkail]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

2677

Oops, should be findRegisterUseOperandIdx(RegNo) < 0 -> !readsRegister(RegNo)

[shchenz]

Thanks for your review @lkail The nits are updated in the commit

[hans]

Pushed to 11.x as 8cf2c031632f88a44eea68b48235496cf1c5d6ec. Please let me know if there are any follow-ups.

llvm/lib/Target/PowerPC/PPCInstrInfo.h

bool isValidToBeChangedReg(MachineInstr *ADDMI, unsigned Index,
                              int64_t OffsetImm) const;
 
   /// Fixup killed/dead flag for register \p RegNo between instructions [\p
   /// StartMI, \p EndMI]. Some pre-RA or post-RA transformations may violate
   /// register killed/dead flags semantics, this function can be called to fix
   /// up. Before calling this function,
   /// 1. Ensure that \p RegNo liveness is killed after instruction \p EndMI.
   /// 2. Ensure that there is no new definition between (\p StartMI, \p EndMI)
-  ///    and possible definition for \p RegNo is \p StartMI or \p EndMI.
+  ///    and possible definition for \p RegNo is \p StartMI or \p EndMI. For
+  ///    pre-RA cases, definition may be \p StartMI through COPY, \p StartMI
+  ///    will be adjust to true definition.
   /// 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,
+  void fixupIsDeadOrKill(MachineInstr *StartMI, MachineInstr *EndMI,
                          unsigned RegNo) 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;
 

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

 const unsigned *PPCInstrInfo::getStoreOpcodesForSpillArray() const {
   return StoreSpillOpcodesArray[getSpillTarget()];
 }
 
 const unsigned *PPCInstrInfo::getLoadOpcodesForSpillArray() const {
   return LoadSpillOpcodesArray[getSpillTarget()];
 }
 
-void PPCInstrInfo::fixupIsDeadOrKill(MachineInstr &StartMI, MachineInstr &EndMI,
+void PPCInstrInfo::fixupIsDeadOrKill(MachineInstr *StartMI, MachineInstr *EndMI,
                                      unsigned RegNo) const {
   // Conservatively clear kill flag for the register if the instructions are in
   // different basic blocks and in SSA form, because the kill flag may no longer
   // be right. There is no need to bother with dead flags since defs with no
   // uses will be handled by DCE.
-  MachineRegisterInfo &MRI = StartMI.getParent()->getParent()->getRegInfo();
-  if (MRI.isSSA() && (StartMI.getParent() != EndMI.getParent())) {
+  MachineRegisterInfo &MRI = StartMI->getParent()->getParent()->getRegInfo();
+  if (MRI.isSSA() && (StartMI->getParent() != EndMI->getParent())) {
     MRI.clearKillFlags(RegNo);
     return;
   }
 
   // Instructions between [StartMI, EndMI] should be in same basic block.
-  assert((StartMI.getParent() == EndMI.getParent()) &&
+  assert((StartMI->getParent() == EndMI->getParent()) &&
          "Instructions are not in same basic block");
 
+  // If before RA, StartMI may be def through COPY, we need to adjust it to the
+  // real def. See function getForwardingDefMI.
+  if (MRI.isSSA()) {
+    bool Reads, Writes;
+    std::tie(Reads, Writes) = StartMI->readsWritesVirtualRegister(RegNo);
+    if (!Reads && !Writes) {
+      assert(Register::isVirtualRegister(RegNo) &&
+             "Must be a virtual register");
+      // Get real def and ignore copies.
+      StartMI = MRI.getVRegDef(RegNo);
+    }
+  }
+
   bool IsKillSet = false;
 
   auto clearOperandKillInfo = [=] (MachineInstr &MI, unsigned Index) {
     MachineOperand &MO = MI.getOperand(Index);
     if (MO.isReg() && MO.isUse() && MO.isKill() &&
         getRegisterInfo().regsOverlap(MO.getReg(), RegNo))
       MO.setIsKill(false);
   };
 
   // Set killed flag for EndMI.
   // No need to do anything if EndMI defines RegNo.
   int UseIndex =
-      EndMI.findRegisterUseOperandIdx(RegNo, false, &getRegisterInfo());
+      EndMI->findRegisterUseOperandIdx(RegNo, false, &getRegisterInfo());
   if (UseIndex != -1) {
-    EndMI.getOperand(UseIndex).setIsKill(true);
+    EndMI->getOperand(UseIndex).setIsKill(true);
     IsKillSet = true;
     // Clear killed flag for other EndMI operands related to RegNo. In some
     // upexpected cases, killed may be set multiple times for same register
     // operand in same MI.
-    for (int i = 0, e = EndMI.getNumOperands(); i != e; ++i)
+    for (int i = 0, e = EndMI->getNumOperands(); i != e; ++i)
       if (i != UseIndex)
-        clearOperandKillInfo(EndMI, i);
+        clearOperandKillInfo(*EndMI, i);
   }
 
   // Walking the inst in reverse order (EndMI -> StartMI].
-  MachineBasicBlock::reverse_iterator It = EndMI;
-  MachineBasicBlock::reverse_iterator E = EndMI.getParent()->rend();
+  MachineBasicBlock::reverse_iterator It = *EndMI;
+  MachineBasicBlock::reverse_iterator E = EndMI->getParent()->rend();
   // EndMI has been handled above, skip it here.
   It++;
   MachineOperand *MO = nullptr;
   for (; It != E; ++It) {
     // Skip insturctions which could not be a def/use of RegNo.
     if (It->isDebugInstr() || It->isPosition())
       continue;
 
if (!IsKillSet) {
       if ((MO = It->findRegisterUseOperand(RegNo, false, &getRegisterInfo()))) {
         // Use found, set it killed.
         IsKillSet = true;
         MO->setIsKill(true);
         continue;
       } else if ((MO = It->findRegisterDefOperand(RegNo, false, true,
                                                   &getRegisterInfo()))) {
         // No use found, set dead for its def.
-        assert(&*It == &StartMI && "No new def between StartMI and EndMI.");
+        assert(&*It == StartMI && "No new def between StartMI and EndMI.");
         MO->setIsDead(true);
         break;
       }
     }
 
-    if ((&*It) == &StartMI)
+    if ((&*It) == StartMI)
       break;
   }
   // Ensure RegMo liveness is killed after EndMI.
   assert((IsKillSet || (MO && MO->isDead())) &&
          "RegNo should be killed or dead");
 }
 
 // This opt tries to convert the following imm form to an index form to save an
if (KilledDef && SetCR)
       *KilledDef = nullptr;
     replaceInstrWithLI(MI, LII);
 
     // Fixup killed/dead flag after transformation.
     // Pattern:
     // ForwardingOperandReg = LI imm1
     // y = op2 imm2, ForwardingOperandReg(killed)
     if (IsForwardingOperandKilled)
-      fixupIsDeadOrKill(DefMI, MI, ForwardingOperandReg);
+      fixupIsDeadOrKill(&DefMI, &MI, ForwardingOperandReg);
 
     LLVM_DEBUG(dbgs() << "With:\n");
     LLVM_DEBUG(MI.dump());
     return true;
   }
   return false;
 }
 
auto IsKilledFor = [&](unsigned Reg) {
         if (It->killsRegister(Reg))
           return true;
       }
       return false;
     };
 
     // Update kill flag
     if (RegMO->isKill() || IsKilledFor(RegMO->getReg()))
-      fixupIsDeadOrKill(DefMI, MI, RegMO->getReg());
+      fixupIsDeadOrKill(&DefMI, &MI, RegMO->getReg());
     if (ForwardKilledOperandReg != ~0U)
-      fixupIsDeadOrKill(DefMI, MI, ForwardKilledOperandReg);
+      fixupIsDeadOrKill(&DefMI, &MI, 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
bool PPCInstrInfo::transformToImmFormFedByAdd(
   // Pattern 1:
   // x = ADD KilledFwdFeederReg, imm
   // n = opn KilledFwdFeederReg(killed), regn
   // y = XOP 0, x
   // Pattern 2:
   // x = ADD reg(killed), imm
   // y = XOP 0, x
   if (IsFwdFeederRegKilled || RegMO->isKill())
-    fixupIsDeadOrKill(DefMI, MI, RegMO->getReg());
+    fixupIsDeadOrKill(&DefMI, &MI, RegMO->getReg());
   // Pattern 3:
   // ForwardKilledOperandReg = ADD reg, imm
   // y = XOP 0, ForwardKilledOperandReg(killed)
   if (ForwardKilledOperandReg != ~0U)
-    fixupIsDeadOrKill(DefMI, MI, ForwardKilledOperandReg);
+    fixupIsDeadOrKill(&DefMI, &MI, ForwardKilledOperandReg);
 
   LLVM_DEBUG(dbgs() << "With:\n");
   LLVM_DEBUG(MI.dump());
 
   return true;
 }
 
 bool PPCInstrInfo::transformToImmFormFedByLI(MachineInstr &MI,
if (!PostRA && III.ZeroIsSpecialOrig != III.ZeroIsSpecialNew) {
     }
   }
 
   // Fix up killed/dead flag after transformation.
   // Pattern:
   // ForwardKilledOperandReg = LI imm
   // y = XOP reg, ForwardKilledOperandReg(killed)
   if (ForwardKilledOperandReg != ~0U)
-    fixupIsDeadOrKill(DefMI, MI, ForwardKilledOperandReg);
+    fixupIsDeadOrKill(&DefMI, &MI, ForwardKilledOperandReg);
   return true;
 }
 
 const TargetRegisterClass *
 PPCInstrInfo::updatedRC(const TargetRegisterClass *RC) const {
   if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)
     return &PPC::VSRCRegClass;
   return RC;

llvm/test/CodeGen/PowerPC/fixup-kill-dead-flag-crash.mir

body: |
 
   bb.1:
     %3:gprc = COPY %2.sub_32:g8rc
     ; CHECK: %4:gprc = LI 0
     %4:gprc = XORI killed %3:gprc, 63
     STW killed %4:gprc, %4:gprc, 100
     BLR8 implicit $lr8, implicit $rm
 ...
+---
+name: test2
+#CHECK : name : test2
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $r3
+    %0:gprc = COPY $r3
+    %1:gprc_and_gprc_nor0 = LI 0
+    ; CHECK: dead %2:gprc = COPY %1
+    %2:gprc = COPY %1:gprc_and_gprc_nor0
+    ; CHECK: %3:gprc = LI 1
+    %3:gprc = ORI killed %2:gprc, 1
+    ; CHECK: STW killed %3, %0, 100
+    STW killed %3:gprc, %0:gprc, 100
+    BLR8 implicit $lr8, implicit $rm
+...