[PowerPC] Remove redundant CRSET/CRUNSET in custom lowering of known CR bit spills

[Yi-Hong.Lyu]

Summary

We lower known CR bit spills (CRSET/CRUNSET) to load and spill the known value
but forgot to remove the redundant spills.

e.g., This sequence was used to spill a CRUNSET:

crclr   4*cr5+lt
mfocrf  r3,4
rlwinm  r3,r3,20,0,0
stw     r3,132(r1)

Custom lowering of known CR bit spills lower it to:

crclr   4*cr5+lt
li        r3,0
stw    r3,132(r1)

crxor is redundant if there is no use of 4*cr5+lt so we should remove it

[Yi-Hong.Lyu]

ping ...

[amyk]

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

759

Maybe we can add an extra comment here, like how there is a comment above the modifiesRegister() call?

[Yi-Hong.Lyu]

Address Amy's comment

[nemanjai]

I believe we could end up with run-time errors (SIGILL) if we get rid of a CRBIT spill and build with -ppc-late-peephole=false. As a result, we need to make sure we get rid of any of those nops we may have introduced.

llvm/lib/Target/PowerPC/PPCInstrInfo.td

3180

Please add a comment here along the lines of:

// Pseudo-instruction marked for deletion. When deleting the instruction
// would cause iterator invalidation in MIR transformation passes, this
// pseudo can be used instead. It will be removed unconditionally at
// pre-emit time (prior to branch selection).

llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp

164–172

We need to change this to add a loop that only gets rid of UNENCODED_NOP instructions. Regardless of whether we want any of the transformations in this pass to run, we definitely need to get rid of those. If we are skipping the rest of the transformations, then we just need to get rid of these nops.

[Yi-Hong.Lyu]

Address Nemanja's comments

[nemanjai]

LGTM. Feel free to address the comments on the commit.

llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp

165

Nit: Expand this comment to:

// Remove UNENCODED_NOP even when this pass is disabled.
// This needs to be done unconditionally so we don't emit zeros
// in the instruction stream.

llvm/test/CodeGen/PowerPC/knowCRBitSpill.ll

6

Please add --implicit-check-not=creqv --implicit-check-not=crxor as we want to make sure we don't emit the CRSET/CRUNSET.

[Yi-Hong.Lyu]

Pushed here https://github.com/llvm/llvm-project/commit/a3db9c08ebdf1f39ed89f4a7afa09fc153cf98c5. Close it manually since I misuse r as in Differential Revision.

[Yi-Hong.Lyu]

BTW, all comments are addressed and updated accordingly before push

llvm/lib/Target/PowerPC/PPCInstrInfo.td

def ADDIStocHA : PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc_nor0:$reg, tocentry32:$disp),
                        "#ADDIStocHA",
                        [(set i32:$rD,
                          (PPCtoc_entry i32:$reg, tglobaladdr:$disp))]>;
 
 // Get Global (GOT) Base Register offset, from the word immediately preceding
 // the function label.
 def UpdateGBR : PPCEmitTimePseudo<(outs gprc:$rD, gprc:$rT), (ins gprc:$rI), "#UpdateGBR", []>;
 
+// Pseudo-instruction marked for deletion. When deleting the instruction would
+// cause iterator invalidation in MIR transformation passes, this pseudo can be
+// used instead. It will be removed unconditionally at pre-emit time (prior to
+// branch selection).
+def UNENCODED_NOP: PPCEmitTimePseudo<(outs), (ins), "#UNENCODED_NOP", []>;
 
 // Standard shifts.  These are represented separately from the real shifts above
 // so that we can distinguish between shifts that allow 5-bit and 6-bit shift
 // amounts.
 def : Pat<(sra i32:$rS, i32:$rB),
           (SRAW $rS, $rB)>;
 def : Pat<(srl i32:$rS, i32:$rB),
           (SRW $rS, $rB)>;

llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp

bool removeRedundantLIs(MachineBasicBlock &MBB,
       for (MachineInstr *MI : InstrsToErase) {
         MI->eraseFromParent();
       }
       NumRemovedInPreEmit += InstrsToErase.size();
       return !InstrsToErase.empty();
     }
 
     bool runOnMachineFunction(MachineFunction &MF) override {
-      if (skipFunction(MF.getFunction()) || !RunPreEmitPeephole)
+      if (skipFunction(MF.getFunction()) || !RunPreEmitPeephole) {
+        // Remove UNENCODED_NOP even with -ppc-late-peephole=false
+        SmallVector<MachineInstr *, 4> InstrsToErase;
+        for (MachineBasicBlock &MBB : MF)
+          for (MachineInstr &MI : MBB)
+            if (MI.getOpcode() == PPC::UNENCODED_NOP)
+              InstrsToErase.push_back(&MI);
+        for (MachineInstr *MI : InstrsToErase)
+          MI->eraseFromParent();
         return false;
+      }
       bool Changed = false;
       const PPCInstrInfo *TII = MF.getSubtarget<PPCSubtarget>().getInstrInfo();
       const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
       SmallVector<MachineInstr *, 4> InstrsToErase;
       for (MachineBasicBlock &MBB : MF) {
         Changed |= removeRedundantLIs(MBB, TRI);
         for (MachineInstr &MI : MBB) {
           unsigned Opc = MI.getOpcode();
+          if (Opc == PPC::UNENCODED_NOP) {
+            InstrsToErase.push_back(&MI);
+            continue;
+          }
           // Detect self copies - these can result from running AADB.
           if (PPCInstrInfo::isSameClassPhysRegCopy(Opc)) {
             const MCInstrDesc &MCID = TII->get(Opc);
             if (MCID.getNumOperands() == 3 &&
                 MI.getOperand(0).getReg() == MI.getOperand(1).getReg() &&
                 MI.getOperand(0).getReg() == MI.getOperand(2).getReg()) {
               NumberOfSelfCopies++;
               LLVM_DEBUG(dbgs() << "Deleting self-copy instruction: ");

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

void PPCRegisterInfo::lowerCRBitSpilling(MachineBasicBlock::iterator II,
   bool LP64 = TM.isPPC64();
   const TargetRegisterClass *G8RC = &PPC::G8RCRegClass;
   const TargetRegisterClass *GPRC = &PPC::GPRCRegClass;
 
   Register Reg = MF.getRegInfo().createVirtualRegister(LP64 ? G8RC : GPRC);
   Register SrcReg = MI.getOperand(0).getReg();
 
   // Search up the BB to find the definition of the CR bit.
-  MachineBasicBlock::reverse_iterator Ins;
+  MachineBasicBlock::reverse_iterator Ins = MI;
+  MachineBasicBlock::reverse_iterator Rend = MBB.rend();
+  ++Ins;
   unsigned CRBitSpillDistance = 0;
-  for (Ins = MI; Ins != MBB.rend(); Ins++) {
+  bool SeenUse = false;
+  for (; Ins != Rend; ++Ins) {
     // Definition found.
     if (Ins->modifiesRegister(SrcReg, TRI))
       break;
+    // Use found.
+    if (Ins->readsRegister(SrcReg, TRI))
+      SeenUse = true;
     // Unable to find CR bit definition within maximum search distance.
     if (CRBitSpillDistance == MaxCRBitSpillDist) {
       Ins = MI;
       break;
     }
     // Skip debug instructions when counting CR bit spill distance.
     if (!Ins->isDebugInstr())
       CRBitSpillDistance++;
   }
 
   // Unable to find the definition of the CR bit in the MBB.
   if (Ins == MBB.rend())
     Ins = MI;
 
+  bool SpillsKnownBit = false;
   // There is no need to extract the CR bit if its value is already known.
   switch (Ins->getOpcode()) {
   case PPC::CRUNSET:
     BuildMI(MBB, II, dl, TII.get(LP64 ? PPC::LI8 : PPC::LI), Reg)
       .addImm(0);
+    SpillsKnownBit = true;
     break;
   case PPC::CRSET:
     BuildMI(MBB, II, dl, TII.get(LP64 ? PPC::LIS8 : PPC::LIS), Reg)
       .addImm(-32768);
+    SpillsKnownBit = true;
     break;
   default:
     // We need to move the CR field that contains the CR bit we are spilling.
     // The super register may not be explicitly defined (i.e. it can be defined
     // by a CR-logical that only defines the subreg) so we state that the CR
     // field is undef. Also, in order to preserve the kill flag on the CR bit,
     // we add it as an implicit use.
     BuildMI(MBB, II, dl, TII.get(LP64 ? PPC::MFOCRF8 : PPC::MFOCRF), Reg)
BuildMI(MBB, II, dl, TII.get(LP64 ? PPC::RLWINM8 : PPC::RLWINM), Reg)
       .addReg(Reg1, RegState::Kill)
       .addImm(getEncodingValue(SrcReg))
       .addImm(0).addImm(0);
   }
   addFrameReference(BuildMI(MBB, II, dl, TII.get(LP64 ? PPC::STW8 : PPC::STW))
                     .addReg(Reg, RegState::Kill),
                     FrameIndex);
 
+  bool KillsCRBit = MI.killsRegister(SrcReg, TRI);
   // Discard the pseudo instruction.
   MBB.erase(II);
+  if (SpillsKnownBit && KillsCRBit && !SeenUse) {
+    Ins->setDesc(TII.get(PPC::UNENCODED_NOP));
+    Ins->RemoveOperand(0);
+  }
 }
 
 void PPCRegisterInfo::lowerCRBitRestore(MachineBasicBlock::iterator II,
                                       unsigned FrameIndex) const {
   // Get the instruction.
   MachineInstr &MI = *II;       // ; <DestReg> = RESTORE_CRBIT <offset>
   // Get the instruction's basic block.
   MachineBasicBlock &MBB = *MI.getParent();

llvm/test/CodeGen/PowerPC/knowCRBitSpill.ll

 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
 ; RUN:     -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr8 \
 ; RUN:     -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:     -ppc-late-peephole=false -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | FileCheck %s
 
 
 ; For known CRBit spills, CRSET/CRUNSET, it is more efficient to just load and
 ; spill the known value.  These tests verify that for CRSET and CRUNSET spills
 ; we do not extract the bit for spilling.
 
 %struct.anon = type { i32 }
 
 @b = common dso_local global %struct.anon* null, align 8
 @a = common dso_local global i64 0, align 8
 
 ; Function Attrs: nounwind
 define dso_local signext i32 @spillCRSET(i32 signext %p1, i32 signext %p2) {
 ; CHECK-LABEL: spillCRSET:
 ; CHECK:        # %bb.2:
 ; CHECK-DAG:    crnor [[CREG:.*]]*cr5+lt, eq, eq
 ; CHECK-DAG:    mfocrf [[REG2:.*]], [[CREG]]
 ; CHECK-DAG:    rlwinm [[REG2]], [[REG2]]
 ; CHECK:        .LBB0_3:
-; CHECK-DAG:    creqv [[CREG:.*]]*cr5+lt, [[CREG]]*cr5+lt, [[CREG]]*cr5+lt
+; CHECK-NOT:    #UNENCODED_NOP
 ; CHECK:        lis [[REG1:.*]], -32768
 ; CHECK:        .LBB0_4:
 ; CHECK-NOT:    mfocrf [[REG2:.*]], [[CREG]]
 ; CHECK-NOT:    rlwinm [[REG2]], [[REG2]]
 ; CHECK:        stw [[REG1]]
 ; CHECK:        # %bb.5:
 
 entry:

 }
 
 %struct.p5rx = type { i32 }
 
 ; Function Attrs: nounwind
 define dso_local signext i32 @spillCRUNSET(%struct.p5rx* readonly %p1, i32 signext %p2, i32 signext %p3) {
 ; CHECK-LABEL: spillCRUNSET:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-DAG:    crxor [[CREG:.*]]*cr5+lt, [[CREG]]*cr5+lt, [[CREG]]*cr5+lt
 ; CHECK-DAG:    li [[REG1:.*]], 0
+; CHECK-NOT:    #UNENCODED_NOP
 ; CHECK-NOT:    mfocrf [[REG2:.*]], [[CREG]]
 ; CHECK-NOT:    rlwinm [[REG2]], [[REG2]]
 ; CHECK:        stw [[REG1]]
 ; CHECK:        .LBB1_1:
 entry:
   %and = and i32 %p3, 128
   %tobool = icmp eq i32 %and, 0
   %tobool2 = icmp eq %struct.p5rx* %p1, null