[RDA] Fix DBG_VALUE issues

[samparker]

Summary

We skip debug instructions in RDA so we cannot attempt to look them up in our instruction map without causing a crash. But some of the methods select the last instruction in the block and this instruction may be a debug instruction... So, use getLastNonDebugInstr instead of calling back on a MachineBasicBlock.
MachineBasicBlock iterators have also been updated to use instructionsWithoutDebug so we can avoid the manual checks for debug instructions.
There's no test attached here... trying to make a sensible reproducer has been a real pain...

[nikic]

This LGTM, with some thoughts on how this could be improved in a followup.

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

501

The general approach in this function looks a bit dubious to me. Instead of passing in the last instruction to getReachingDef(), we should add a function that gets the reaching def at the end of the block. This can be found by inspecting the last element in the reaching def vector, rather than scanning the whole vector, and has the advantage that it will automatically include reaching defs from the last instruction. From a cursory look, I suspect that the current implementation has a discrepancy in handling of aliased registers for the last instruction, because reg units are not inspected for it.

[samparker]

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

501

Thanks, sounds good.

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

 }
 
 void ReachingDefAnalysis::reprocessBasicBlock(MachineBasicBlock *MBB) {
   unsigned MBBNumber = MBB->getNumber();
   assert(MBBNumber < MBBReachingDefs.size() &&
          "Unexpected basic block number.");
 
   // Count number of non-debug instructions for end of block adjustment.
-  int NumInsts = 0;
-  for (const MachineInstr &MI : *MBB)
-    if (!MI.isDebugInstr())
-      NumInsts++;
+  auto NonDbgInsts =
+    instructionsWithoutDebug(MBB->instr_begin(), MBB->instr_end());
+  int NumInsts = std::distance(NonDbgInsts.begin(), NonDbgInsts.end());
 
   // When reprocessing a block, the only thing we need to do is check whether
   // there is now a more recent incoming reaching definition from a predecessor.
   for (MachineBasicBlock *pred : MBB->predecessors()) {
     assert(unsigned(pred->getNumber()) < MBBOutRegsInfos.size() &&
            "Should have pre-allocated MBBInfos for all MBBs");
     const LiveRegsDefInfo &Incoming = MBBOutRegsInfos[pred->getNumber()];
     // Incoming may be empty for dead predecessors.
void ReachingDefAnalysis::processBasicBlock(
 
   if (!TraversedMBB.PrimaryPass) {
     // Reprocess MBB that is part of a loop.
     reprocessBasicBlock(MBB);
     return;
   }
 
   enterBasicBlock(MBB);
-  for (MachineInstr &MI : *MBB) {
-    if (!MI.isDebugInstr())
-      processDefs(&MI);
-  }
+  for (MachineInstr &MI :
+       instructionsWithoutDebug(MBB->instr_begin(), MBB->instr_end()))
   leaveBasicBlock(MBB);
 }
 
 bool ReachingDefAnalysis::runOnMachineFunction(MachineFunction &mf) {
   MF = &mf;
   TRI = MF->getSubtarget().getRegisterInfo();
   LLVM_DEBUG(dbgs() << "********** REACHING DEFINITION ANALYSIS **********\n");
   init();
for (auto &MO : MI->operands()) {
         return;
     }
   }
 }
 
 bool
 ReachingDefAnalysis::getLiveInUses(MachineBasicBlock *MBB, int PhysReg,
                                    InstSet &Uses) const {
-  for (auto &MI : *MBB) {
-    if (MI.isDebugInstr())
-      continue;
+  for (MachineInstr &MI :
+       instructionsWithoutDebug(MBB->instr_begin(), MBB->instr_end())) {
     for (auto &MO : MI.operands()) {
       if (!isValidRegUseOf(MO, PhysReg))
         continue;
       if (getReachingDef(&MI, PhysReg) >= 0)
         return false;
       Uses.insert(&MI);
     }
   }
-  return isReachingDefLiveOut(&MBB->back(), PhysReg);
+  MachineInstr *Last = &*MBB->getLastNonDebugInstr();
+  return isReachingDefLiveOut(Last, PhysReg);
 }
 
 void
 ReachingDefAnalysis::getGlobalUses(MachineInstr *MI, int PhysReg,
                                    InstSet &Uses) const {
   MachineBasicBlock *MBB = MI->getParent();
 
   // Collect the uses that each def touches within the block.
bool ReachingDefAnalysis::isRegUsedAfter(MachineInstr *MI, int PhysReg) const {
   LiveRegs.addLiveOuts(*MBB);
 
   // Yes if the register is live out of the basic block.
   if (LiveRegs.contains(PhysReg))
     return true;
 
   // Walk backwards through the block to see if the register is live at some
   // point.
-  for (auto Last = MBB->rbegin(), End = MBB->rend(); Last != End; ++Last) {
-    LiveRegs.stepBackward(*Last);
+  for (MachineInstr &Last :
+       instructionsWithoutDebug(MBB->instr_rbegin(), MBB->instr_rend())) {
+    LiveRegs.stepBackward(Last);
     if (LiveRegs.contains(PhysReg))
-      return InstIds.lookup(&*Last) > InstIds.lookup(MI);
+      return InstIds.lookup(&Last) > InstIds.lookup(MI);
   }
   return false;
 }
 
 bool ReachingDefAnalysis::isRegDefinedAfter(MachineInstr *MI,
                                             int PhysReg) const {
   MachineBasicBlock *MBB = MI->getParent();
-  if (getReachingDef(MI, PhysReg) != getReachingDef(&MBB->back(), PhysReg))
+  MachineInstr *Last = &*MBB->getLastNonDebugInstr();
+  if (getReachingDef(MI, PhysReg) != getReachingDef(Last, PhysReg))
     return true;
 
   if (auto *Def = getLocalLiveOutMIDef(MBB, PhysReg))
     return Def == getReachingLocalMIDef(MI, PhysReg);
 
   return false;
 }
 
 bool
 ReachingDefAnalysis::isReachingDefLiveOut(MachineInstr *MI, int PhysReg) const {
   MachineBasicBlock *MBB = MI->getParent();
   LivePhysRegs LiveRegs(*TRI);
   LiveRegs.addLiveOuts(*MBB);
   if (!LiveRegs.contains(PhysReg))
     return false;
 
-  MachineInstr *Last = &MBB->back();
+  MachineInstr *Last = &*MBB->getLastNonDebugInstr();
   int Def = getReachingDef(MI, PhysReg);
   if (getReachingDef(Last, PhysReg) != Def)
     return false;
 
   // Finally check that the last instruction doesn't redefine the register.
   for (auto &MO : Last->operands())
     if (isValidRegDefOf(MO, PhysReg))
       return false;
 
   return true;
 }
 
 MachineInstr* ReachingDefAnalysis::getLocalLiveOutMIDef(MachineBasicBlock *MBB,
                                                         int PhysReg) const {
   LivePhysRegs LiveRegs(*TRI);
   LiveRegs.addLiveOuts(*MBB);
   if (!LiveRegs.contains(PhysReg))
     return nullptr;
 
-  MachineInstr *Last = &MBB->back();
+  MachineInstr *Last = &*MBB->getLastNonDebugInstr();
   int Def = getReachingDef(Last, PhysReg);
   for (auto &MO : Last->operands())
     if (isValidRegDefOf(MO, PhysReg))
       return Last;
 
   return Def < 0 ? nullptr : getInstFromId(MBB, Def);
 }