[PowerPC] start and end parameters for fixupIsDeadOrKill may exist in different block before RA

[shchenz]

Summary

In fixupIsDeadOrKill, we assume StartMI and EndMI not exist in same basic block, so we add an assertion in that function.This is right after RA.

We extend this assumption to before RA in https://reviews.llvm.org/D81723. This is not right, as before RA the true definition may exist in another block through copy like instructions.

[shchenz]

update according to @nemanjai offline comments

[shchenz]

Hi @nemanjai Thanks for your offline comments. could you please help to have another look at this issue?

[shchenz]

gentle ping

[nemanjai]

LGTM. I am really sorry about not getting back to this earlier.

[shchenz]

Thanks for your review. ^-^

llvm/lib/Target/PowerPC/PPCInstrInfo.h

bool isImmInstrEligibleForFolding(MachineInstr &MI, unsigned &BaseReg,
                                     unsigned &XFormOpcode,
                                     int64_t &OffsetOfImmInstr,
                                     ImmInstrInfo &III) const;
   bool isValidToBeChangedReg(MachineInstr *ADDMI, unsigned Index,
                              MachineInstr *&ADDIMI, int64_t &OffsetAddi,
                              int64_t OffsetImm) const;
 
   /// Fixup killed/dead flag for register \p RegNo between instructions [\p
-  /// StartMI, \p EndMI]. Some PostRA transformations may violate register
-  /// killed/dead flags semantics, this function can be called to fix up. Before
-  /// calling this function,
+  /// 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.
-  /// 3. Ensure that all instructions between [\p StartMI, \p EndMI] are in same
-  ///    basic block.
+  /// 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;
   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::getLoadOpcodesForSpillArray() const {
   return LoadSpillOpcodesArray[getSpillTarget()];
 }
 
 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())) {
+    MRI.clearKillFlags(RegNo);
+    return;
+  }
 
   // Instructions between [StartMI, EndMI] should be in same basic block.
   assert((StartMI.getParent() == EndMI.getParent()) &&
          "Instructions are not in same basic block");
 
   bool IsKillSet = false;
 
   auto clearOperandKillInfo = [=] (MachineInstr &MI, unsigned Index) {

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

+# RUN: llc -mtriple=powerpc64le-unknown-linux-gnu -verify-machineinstrs -start-before ppc-mi-peepholes \
+# RUN:   -stop-after ppc-mi-peepholes %s -o - | FileCheck %s
+
+---
+name: test
+#CHECK : name : test
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $x3
+    %0:g8rc = COPY $x3
+    %1:gprc = COPY %0.sub_32:g8rc
+    %2:g8rc = LI8 63
+
+  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
+...