[RDA] Track implicit-defs

[samparker]

Summary

Ensure that we're recording implicit defs, as well as visiting implicit uses and implicit defs when we're walking through operands. This requires a change to ARMLowOverheadLoops because RDA now knows that t2LoopEnd is set to define the CPSR.

[samparker]

Rebased for new test case.

[SjoerdMeijer]

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

105

I am thinking if it is time for a MO helper function that tests this, because I see a lot of the same/similar checks:

if (!MO.isReg() || !MO.isUse() || MO.getReg() != PhysReg)
if (!MO.isReg() || !MO.isUse() || MO.getReg() != PhysReg)
if (MO.isReg() && MO.isDef() && MO.getReg() == PhysReg)
if (MO.isReg() && MO.isDef() && MO.getReg() == PhysReg)
if (!MO.isReg() || MO.isUndef() || !MO.getReg())
if (MO.isReg() && MO.getReg() && Defs.count(MO.getReg()))
if (!MO.isReg() || MO.isUse() || MO.getReg() == 0)
if (!MO.isReg() || MO.getReg() == 0 || !MO.isKill())
if (MO.isReg() && MO.isDef() && MO.getReg() == PhysReg)

Perhaps a local static helper function(s) is easiest to increase readability?

[samparker]

Adding a few helpers for use/defs.

[SjoerdMeijer]

Thanks for that! That greatly helps readability!

Where in this patch stack do we expect test changes?

[samparker]

Rebased after reverting to show the test changes that we want.

[SjoerdMeijer]

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp

820

Do we need to start looking again for t2LoopEnd? Is that not saved to End, do we have access to that?

[samparker]

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp

820

We still need this because t2LoopEnd isn't actually the last instruction, there's almost certainly an unconditional branch as the terminator, so this code was originally not doing what is was supposed to! Now that we're correctly tracking implicit-defs, RDA will pick up that !isSafeToDefRegAt because t2LoopEnd has an implicit CPSR def and we definitely want to ignore it.

[SjoerdMeijer]

Thanks, LGTM

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

 }
 
 void ReachingDefAnalysis::processDefs(MachineInstr *MI) {
   assert(!MI->isDebugInstr() && "Won't process debug instructions");
 
   unsigned MBBNumber = MI->getParent()->getNumber();
   assert(MBBNumber < MBBReachingDefs.size() &&
          "Unexpected basic block number.");
-  const MCInstrDesc &MCID = MI->getDesc();
-  for (unsigned i = 0,
-                e = MI->isVariadic() ? MI->getNumOperands() : MCID.getNumDefs();
-       i != e; ++i) {
-    MachineOperand &MO = MI->getOperand(i);
-    if (!MO.isReg() || !MO.getReg())
-      continue;
-    if (MO.isUse())
+
+  for (auto &MO : MI->operands()) {
+    if (!MO.isReg() || !MO.getReg() || MO.isUse())
       continue;
     for (MCRegUnitIterator Unit(MO.getReg(), TRI); Unit.isValid(); ++Unit) {
       // This instruction explicitly defines the current reg unit.
       LLVM_DEBUG(dbgs() << printReg(MO.getReg(), TRI) << ":\t" << CurInstr
                         << '\t' << *MI);
 
       // How many instructions since this reg unit was last written?
       LiveRegs[*Unit] = CurInstr;
ReachingDefAnalysis::isSafeToRemove(MachineInstr *MI, InstSet &Visited,
   return true;
 }
 
 void ReachingDefAnalysis::collectLocalKilledOperands(MachineInstr *MI,
                                                      InstSet &Dead) const {
   Dead.insert(MI);
   auto IsDead = [this](MachineInstr *Def, int PhysReg) {
     unsigned LiveDefs = 0;
-    for (auto &MO : Def->defs())
+    for (auto &MO : Def->operands()) {
+      if (!MO.isReg() || !MO.getReg() || MO.isUse())
+        continue;
       if (!MO.isDead())
         ++LiveDefs;
+    }
 
     if (LiveDefs > 1)
       return false;
 
     SmallPtrSet<MachineInstr*, 4> Uses;
     getGlobalUses(Def, PhysReg, Uses);
     return Uses.size() == 1;
   };
 
-  for (auto &MO : MI->uses()) {
-    if (!MO.isReg() || MO.getReg() == 0 || !MO.isKill())
+  for (auto &MO : MI->operands()) {
+    if (!MO.isReg() || !MO.getReg() || !MO.isUse() || !MO.isKill())
       continue;
     if (MachineInstr *Def = getReachingLocalMIDef(MI, MO.getReg()))
       if (IsDead(Def, MO.getReg()))
         collectLocalKilledOperands(Def, Dead);
   }
 }
 
 bool ReachingDefAnalysis::isSafeToDefRegAt(MachineInstr *MI,

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp

   MI->eraseFromParent();
 }
 
 bool ARMLowOverheadLoops::RevertLoopDec(MachineInstr *MI) const {
   LLVM_DEBUG(dbgs() << "ARM Loops: Reverting to sub: " << *MI);
   MachineBasicBlock *MBB = MI->getParent();
   MachineInstr *Last = &MBB->back();
   SmallPtrSet<MachineInstr*, 1> Ignore;
-  if (Last->getOpcode() == ARM::t2LoopEnd)
-    Ignore.insert(Last);
+  for (auto I = MachineBasicBlock::iterator(MI), E = MBB->end(); I != E; ++I) {
+    if (I->getOpcode() == ARM::t2LoopEnd) {
+      Ignore.insert(&*I);
+      break;
+    }
+  }
 
   // If nothing defines CPSR between LoopDec and LoopEnd, use a t2SUBS.
   bool SetFlags = RDA->isSafeToDefRegAt(MI, ARM::CPSR, Ignore);
 
   MachineInstrBuilder MIB = BuildMI(*MBB, MI, MI->getDebugLoc(),
                                     TII->get(ARM::t2SUBri));
   MIB.addDef(ARM::LR);
   MIB.add(MI->getOperand(1));

llvm/test/CodeGen/Thumb2/LowOverheadLoops/dont-ignore-vctp.mir

- id:              0
     value:           '<4 x float> <float 1.000000e+00, float -1.000000e+00, float 1.000000e+00, float -1.000000e+00>'
     alignment:       16
     isTargetSpecific: false
 machineFunctionInfo: {}
 body:             |
   ; CHECK-LABEL: name: dont_ignore_vctp
   ; CHECK: bb.0.entry:
   ; CHECK:   successors: %bb.1(0x80000000)
-  ; CHECK:   liveins: $lr, $r0, $r1, $r3, $r7
+  ; CHECK:   liveins: $lr, $r0, $r1, $r2, $r7
   ; CHECK:   frame-setup tPUSH 14 /* CC::al */, $noreg, killed $r7, killed $lr, implicit-def $sp, implicit $sp
   ; CHECK:   frame-setup CFI_INSTRUCTION def_cfa_offset 8
   ; CHECK:   frame-setup CFI_INSTRUCTION offset $lr, -4
   ; CHECK:   frame-setup CFI_INSTRUCTION offset $r7, -8
-  ; CHECK:   tCMPi8 renamable $r3, 4, 14 /* CC::al */, $noreg, implicit-def dead $cpsr
+  ; CHECK:   renamable $r3, dead $cpsr = tLSLri killed renamable $r2, 1, 14 /* CC::al */, $noreg
   ; CHECK:   renamable $r2 = tLEApcrel %const.0, 14 /* CC::al */, $noreg
   ; CHECK:   renamable $q0 = MVE_VLDRWU32 killed renamable $r2, 0, 0, $noreg :: (load 16 from constant-pool)
   ; CHECK:   $lr = MVE_DLSTP_32 killed renamable $r3
   ; CHECK: bb.1.do.body (align 4):
   ; CHECK:   successors: %bb.1(0x7c000000), %bb.2(0x04000000)
   ; CHECK:   liveins: $lr, $q0, $r0, $r1
   ; CHECK:   renamable $q1 = nnan ninf nsz MVE_VLDRWU32 renamable $r0, 0, 0, $noreg
   ; CHECK:   renamable $q1 = nnan ninf nsz MVE_VMULf32 killed renamable $q1, renamable $q0, 0, $noreg, undef renamable $q1