[DebugInfo] Don't reorder DBG_VALUEs when sunk

Fix part of PR43855, resolving a problem that comes from the reapplication
in 001574938e5. If we have two DBG_VALUE insts in a block that specify
the location of the same variable, for example:

   %0 = someinst
   DBG_VALUE %0, !123, !DIExpression()
   %1 = anotherinst
   DBG_VALUE %1, !123, !DIExpression()

if %0 were to sink, the corresponding DBG_VALUE would sink too, past the
next DBG_VALUE, effectively re-ordering assignments. To fix this, I've
added a SeenDbgVars set recording what variable locations have been seen in
a block already (working bottom up), and now flag DBG_VALUEs that would
pass a later DBG_VALUE for the same variable.

NB, this only works for repeated DBG_VALUEs in the same basic block, the
general case involving control flow is much harder, which I've written
up in PR44117.

Differential revision: https://reviews.llvm.org/D70672

1 files changed, 93 insertions, 51 deletions

diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index f34e820a2e0..6900c195e0e 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -15,6 +15,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
@@ -106,8 +108,24 @@ namespace {
     using AllSuccsCache =
         std::map<MachineBasicBlock *, SmallVector<MachineBasicBlock *, 4>>;
 
-    // Remember debug uses of vregs seen, so we know what to sink out of blocks.
-    DenseMap<unsigned, TinyPtrVector<MachineInstr *>> SeenDbgUsers;
+    /// DBG_VALUE pointer and flag. The flag is true if this DBG_VALUE is
+    /// post-dominated by another DBG_VALUE of the same variable location.
+    /// This is necessary to detect sequences such as:
+    ///     %0 = someinst
+    ///     DBG_VALUE %0, !123, !DIExpression()
+    ///     %1 = anotherinst
+    ///     DBG_VALUE %1, !123, !DIExpression()
+    /// Where if %0 were to sink, the DBG_VAUE should not sink with it, as that
+    /// would re-order assignments.
+    using SeenDbgUser = PointerIntPair<MachineInstr *, 1>;
+
+    /// Record of DBG_VALUE uses of vregs in a block, so that we can identify
+    /// debug instructions to sink.
+    SmallDenseMap<unsigned, TinyPtrVector<SeenDbgUser>> SeenDbgUsers;
+
+    /// Record of debug variables that have had their locations set in the
+    /// current block.
+    DenseSet<DebugVariable> SeenDbgVars;
 
   public:
     static char ID; // Pass identification
@@ -399,6 +417,7 @@ bool MachineSinking::ProcessBlock(MachineBasicBlock &MBB) {
   } while (!ProcessedBegin);
 
   SeenDbgUsers.clear();
+  SeenDbgVars.clear();
 
   return MadeChange;
 }
@@ -408,11 +427,16 @@ void MachineSinking::ProcessDbgInst(MachineInstr &MI) {
   // we know what to sink if the vreg def sinks.
   assert(MI.isDebugValue() && "Expected DBG_VALUE for processing");
 
+  DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
+                    MI.getDebugLoc()->getInlinedAt());
+  bool SeenBefore = SeenDbgVars.count(Var) != 0;
+
   MachineOperand &MO = MI.getOperand(0);
-  if (!MO.isReg() || !MO.getReg().isVirtual())
-    return;
+  if (MO.isReg() && MO.getReg().isVirtual())
+    SeenDbgUsers[MO.getReg()].push_back(SeenDbgUser(&MI, SeenBefore));
 
-  SeenDbgUsers[MO.getReg()].push_back(&MI);
+  // Record the variable for any DBG_VALUE, to avoid re-ordering any of them.
+  SeenDbgVars.insert(Var);
 }
 
 bool MachineSinking::isWorthBreakingCriticalEdge(MachineInstr &MI,
@@ -759,12 +783,60 @@ static bool SinkingPreventsImplicitNullCheck(MachineInstr &MI,
          MBP.LHS.getReg() == BaseOp->getReg();
 }
 
+/// If the sunk instruction is a copy, try to forward the copy instead of
+/// leaving an 'undef' DBG_VALUE in the original location. Don't do this if
+/// there's any subregister weirdness involved. Returns true if copy
+/// propagation occurred.
+static bool attemptDebugCopyProp(MachineInstr &SinkInst, MachineInstr &DbgMI) {
+  const MachineRegisterInfo &MRI = SinkInst.getMF()->getRegInfo();
+  const TargetInstrInfo &TII = *SinkInst.getMF()->getSubtarget().getInstrInfo();
+
+  // Copy DBG_VALUE operand and set the original to undef. We then check to
+  // see whether this is something that can be copy-forwarded. If it isn't,
+  // continue around the loop.
+  MachineOperand DbgMO = DbgMI.getOperand(0);
+
+  const MachineOperand *SrcMO = nullptr, *DstMO = nullptr;
+  auto CopyOperands = TII.isCopyInstr(SinkInst);
+  if (!CopyOperands)
+    return false;
+  SrcMO = CopyOperands->Source;
+  DstMO = CopyOperands->Destination;
+
+  // Check validity of forwarding this copy.
+  bool PostRA = MRI.getNumVirtRegs() == 0;
+
+  // Trying to forward between physical and virtual registers is too hard.
+  if (DbgMO.getReg().isVirtual() != SrcMO->getReg().isVirtual())
+    return false;
+
+  // Only try virtual register copy-forwarding before regalloc, and physical
+  // register copy-forwarding after regalloc.
+  bool arePhysRegs = !DbgMO.getReg().isVirtual();
+  if (arePhysRegs != PostRA)
+    return false;
+
+  // Pre-regalloc, only forward if all subregisters agree (or there are no
+  // subregs at all). More analysis might recover some forwardable copies.
+  if (!PostRA && (DbgMO.getSubReg() != SrcMO->getSubReg() ||
+                  DbgMO.getSubReg() != DstMO->getSubReg()))
+    return false;
+
+  // Post-regalloc, we may be sinking a DBG_VALUE of a sub or super-register
+  // of this copy. Only forward the copy if the DBG_VALUE operand exactly
+  // matches the copy destination.
+  if (PostRA && DbgMO.getReg() != DstMO->getReg())
+    return false;
+
+  DbgMI.getOperand(0).setReg(SrcMO->getReg());
+  DbgMI.getOperand(0).setSubReg(SrcMO->getSubReg());
+  return true;
+}
+
 /// Sink an instruction and its associated debug instructions.
 static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
                         MachineBasicBlock::iterator InsertPos,
                         SmallVectorImpl<MachineInstr *> &DbgValuesToSink) {
-  const MachineRegisterInfo &MRI = MI.getMF()->getRegInfo();
-  const TargetInstrInfo &TII = *MI.getMF()->getSubtarget().getInstrInfo();
 
   // If we cannot find a location to use (merge with), then we erase the debug
   // location to prevent debug-info driven tools from potentially reporting
@@ -784,9 +856,6 @@ static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
   // DBG_VALUE location as 'undef', indicating that any earlier variable
   // location should be terminated as we've optimised away the value at this
   // point.
-  // If the sunk instruction is a copy, try to forward the copy instead of
-  // leaving an 'undef' DBG_VALUE in the original location. Don't do this if
-  // there's any subregister weirdness involved.
   for (SmallVectorImpl<MachineInstr *>::iterator DBI = DbgValuesToSink.begin(),
                                                  DBE = DbgValuesToSink.end();
        DBI != DBE; ++DBI) {
@@ -794,46 +863,8 @@ static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
     MachineInstr *NewDbgMI = DbgMI->getMF()->CloneMachineInstr(*DBI);
     SuccToSinkTo.insert(InsertPos, NewDbgMI);
 
-    // Copy DBG_VALUE operand and set the original to undef. We then check to
-    // see whether this is something that can be copy-forwarded. If it isn't,
-    // continue around the loop.
-    MachineOperand DbgMO = DbgMI->getOperand(0);
-    DbgMI->getOperand(0).setReg(0);
-
-    const MachineOperand *SrcMO = nullptr, *DstMO = nullptr;
-    auto CopyOperands = TII.isCopyInstr(MI);
-    if (!CopyOperands)
-      continue;
-    SrcMO = CopyOperands->Source;
-    DstMO = CopyOperands->Destination;
-
-    // Check validity of forwarding this copy.
-    bool PostRA = MRI.getNumVirtRegs() == 0;
-
-    // Trying to forward between physical and virtual registers is too hard.
-    if (DbgMO.getReg().isVirtual() != SrcMO->getReg().isVirtual())
-      continue;
-
-    // Only try virtual register copy-forwarding before regalloc, and physical
-    // register copy-forwarding after regalloc.
-    bool arePhysRegs = !DbgMO.getReg().isVirtual();
-    if (arePhysRegs != PostRA)
-      continue;
-
-    // Pre-regalloc, only forward if all subregisters agree (or there are no
-    // subregs at all). More analysis might recover some forwardable copies.
-    if (!PostRA && (DbgMO.getSubReg() != SrcMO->getSubReg() ||
-                    DbgMO.getSubReg() != DstMO->getSubReg()))
-      continue;
-
-    // Post-regalloc, we may be sinking a DBG_VALUE of a sub or super-register
-    // of this copy. Only forward the copy if the DBG_VALUE operand exactly
-    // matches the copy destination.
-    if (PostRA && DbgMO.getReg() != DstMO->getReg())
-      continue;
-
-    DbgMI->getOperand(0).setReg(SrcMO->getReg());
-    DbgMI->getOperand(0).setSubReg(SrcMO->getSubReg());
+    if (!attemptDebugCopyProp(MI, *DbgMI))
+      DbgMI->getOperand(0).setReg(0);
   }
 }
 
@@ -959,8 +990,19 @@ bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
     if (!SeenDbgUsers.count(MO.getReg()))
       continue;
 
+    // Sink any users that don't pass any other DBG_VALUEs for this variable.
     auto &Users = SeenDbgUsers[MO.getReg()];
-    DbgUsersToSink.insert(DbgUsersToSink.end(), Users.begin(), Users.end());
+    for (auto &User : Users) {
+      MachineInstr *DbgMI = User.getPointer();
+      if (User.getInt()) {
+        // This DBG_VALUE would re-order assignments. If we can't copy-propagate
+        // it, it can't be recovered. Set it undef.
+        if (!attemptDebugCopyProp(MI, *DbgMI))
+          DbgMI->getOperand(0).setReg(0);
+      } else {
+        DbgUsersToSink.push_back(DbgMI);
+      }
+    }
   }
 
   // After sinking, some debug users may not be dominated any more. If possible,