[MachineCopyPropagation] Fix differences in code gen when compiling with -g

[predator5047]

Summary

Fixes bugs 50580 and 49446

When compiling with -g "DBG_VALUE <reg>" instructions are added in the MIR, if such a instruction is inserted between instructions that use <reg> then MachineCopyPropagation invalidates <reg> , this causes some copies to not be propagated and causes differences in code generation (ex bugs 50580 and 49446 ). DBG_VALUE instructions should be ignored since they don't actually modify the register.

[lattner]

I'm not a competent reviewer for this patch.

[lkail]

There is a patch https://reviews.llvm.org/D98401 doing the same. As discussed in D98401, the register in DBG_VALUE should be also updated. I think backward mcp is missing tracking usage when MO.isDebug() like forward mcp does.

[jmorse]

@predator5047 This patch is worth pursuing -- D98401 involves codegen alterations and will necessarily take a long while to review; wheras this patch could land before llvm-13.

This patch is obviously effective at preventing codegen from being changed by debug-info; the downside is that the variable location is wrong afterwards (DBG_VALUE $rcx in the CHECK lines). I'm fairly confident we would prefer to have a false variable location over unstable code generation so it could land as-is; @aprantl @probinson @dblaikie would you agree?

To get the variable location right, you need to populate the CopyDbgUsers container, and re-write them when propagateDefs deletes the copy.

[predator5047]

@jmorse OK, I will reclaim this revision and make the changes you suggested.

[lkail]

Reverse ping. Hi @predator5047 are you still working on this? One solution might be tracking debug instruction like what MachineCopyPropagation::ReadRegister does.

[predator5047]

Update debug instructions if a register is changed because of backward propagation.

[lkail]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

829

This looks unnecessary, can use findCopyDefViaUnit to find out corresponding MachineInstr.

878

nit:

if (MO.readsReg()) {
  if (MO.isDebug()) {
    ...
  } else {
    ...
  }
}

898

nit: OldR -> Def, NewR -> Src for consistency.

[predator5047]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

829

findCopyDefViaUnit or ReadRegister don't work for backwards propagation, because Tracker.trackCopy sets the CopyInfo::MI filed for the source register to null (line 167), so both would always return null.

[lkail]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

882

-      if (MO.readsReg())
-        Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
+      if (MO.readsReg()) {
+        if (MO.isDebug()) {
+          for (MCRegUnitIterator RUI(MO.getReg(), TRI); RUI.isValid(); ++RUI) {
+            if (MachineInstr *Copy = Tracker.findCopyDefViaUnit(*RUI, *TRI)) {
+              CopyDbgUsers[Copy].insert(MI);
+            }
+          }
+        } else {
+          Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
+        }
+      }

Doesn't it work?

[predator5047]

Implemented lkail suggestions and addressed his comments.

[lkail]

LGTM, thanks for your patience. May be @jmorse has further comments.

[lkail]

Hi @predator5047 do you need help landing this patch?

[predator5047]

Yes, can you commit on my behalf.
name: Butiu Alexandru Octavian
email: alexandru.octavian.butiu at gmail.com

llvm/lib/CodeGen/MachineCopyPropagation.cpp

for (unsigned OpIdx = 0, OpEnd = MI.getNumOperands(); OpIdx != OpEnd;
     ++NumCopyBackwardPropagated;
   }
 }
 
 void MachineCopyPropagation::BackwardCopyPropagateBlock(
     MachineBasicBlock &MBB) {
   LLVM_DEBUG(dbgs() << "MCP: BackwardCopyPropagateBlock " << MBB.getName()
                     << "\n");
+  llvm::DenseMap<llvm::MCRegister, llvm::MachineInstr *> RegToCopies;
 
   for (MachineBasicBlock::reverse_iterator I = MBB.rbegin(), E = MBB.rend();
        I != E;) {
     MachineInstr *MI = &*I;
     ++I;
 
     // Ignore non-trivial COPYs.
     if (MI->isCopy() && MI->getNumOperands() == 2 &&
         !TRI->regsOverlap(MI->getOperand(0).getReg(),
                           MI->getOperand(1).getReg())) {
 
       MCRegister Def = MI->getOperand(0).getReg().asMCReg();
       MCRegister Src = MI->getOperand(1).getReg().asMCReg();
 
       // Unlike forward cp, we don't invoke propagateDefs here,
       // just let forward cp do COPY-to-COPY propagation.
       if (isBackwardPropagatableCopy(*MI, *MRI)) {
         Tracker.invalidateRegister(Src, *TRI);
         Tracker.invalidateRegister(Def, *TRI);
         Tracker.trackCopy(MI, *TRI);
+        // Map registers to copy instructions.
+        for (MCRegUnitIterator RUI(Src, TRI); RUI.isValid(); ++RUI) {
+          RegToCopies[*RUI] = MI;
+        }
         continue;
       }
     }
 
     // Invalidate any earlyclobber regs first.
     for (const MachineOperand &MO : MI->operands())
       if (MO.isReg() && MO.isEarlyClobber()) {
         MCRegister Reg = MO.getReg().asMCReg();
         if (!Reg)
           continue;
         Tracker.invalidateRegister(Reg, *TRI);
       }
 
     propagateDefs(*MI);
     for (const MachineOperand &MO : MI->operands()) {
       if (!MO.isReg())
         continue;
 
       if (!MO.getReg())
         continue;
 
       if (MO.isDef())
         Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
 
-      if (MO.readsReg())
+      if (MO.readsReg() && MO.isDebug()) {
+        //  Check if the register in the debug instruction is utilized
+        // in a copy instruction, so we can update the debug info if the
+        // register is changed.
+        for (MCRegUnitIterator RUI(MO.getReg().asMCReg(), TRI); RUI.isValid();
+             ++RUI) {
+          auto MIIT = RegToCopies.find(*RUI);
+          if (MIIT != RegToCopies.end()) {
+            CopyDbgUsers[MIIT->second].insert(MI);
+          }
+        }
+      } else if (MO.readsReg()) {
         Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
       }
     }
+  }
 
   for (auto *Copy : MaybeDeadCopies) {
+
+    Register NewR = Copy->getOperand(1).getReg();
+    Register OldR = Copy->getOperand(0).getReg();
+    SmallVector<MachineInstr *> MaybeDeadDbgUsers(CopyDbgUsers[Copy].begin(),
+                                                  CopyDbgUsers[Copy].end());
+
+    MRI->updateDbgUsersToReg(NewR.asMCReg(), OldR.asMCReg(), MaybeDeadDbgUsers);
     Copy->eraseFromParent();
     ++NumDeletes;
   }
 
   MaybeDeadCopies.clear();
   CopyDbgUsers.clear();
   Tracker.clear();
 }

llvm/test/CodeGen/X86/machine-copy-dbgvalue.mir

+# RUN: llc -mtriple=i686-- -run-pass machine-cp -verify-machineinstrs -o - %s | FileCheck %s
+
+
+---
+# Test that machine copy propagation ignores DBG_VALUE and DBL_VALUE_LIST and updates it.
+# CHECK-LABEL: name: foo
+# CHECK: bb.0:
+# CHECK-NEXT: $rax = MOV64ri 31
+# CHECK-NEXT: DBG_VALUE $rax
+# CHECK-NEXT: DBG_VALUE_LIST 0, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 4, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), $rax, 0, 0
+# CHECK-NEXT: RETQ implicit killed $rax
+name: foo
+body: |
+  bb.0:
+    renamable $rcx = MOV64ri 31
+    DBG_VALUE $rcx, 0, 0, 0, 0
+    DBG_VALUE_LIST 0, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 4, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), $rcx, 0, 0
+    $rax = COPY killed renamable $rcx
+    RETQ implicit killed $rax
+...