[2/2][LiveDebugVariables] Avoid reduntant DBG_VALUEs after virtregrewrite

[djtodoro]

Summary

This is a follow-up for the D105025. It tries to find duplicates even further.

[Orlando]

Hi @djtodoro,

Thanks for splitting the review into two. If D105025 gets accepted as it is then this one makes sense and LGTM. The patch summary could probably be a little more descriptive though. Maybe something like "Ignore instructions that do not modify the variable location when searching backwards for duplicate DBG_VALUEs, rather than stopping the search"?

Lastly, I think it might be worth having a dedicated test for this - or if not, at least adding a comment into the modified test so that it's clear that this feature is being tested. I'm worried that the test could change or go away without us noticing and we'd lose the testing coverage. wdyt?

llvm/lib/CodeGen/LiveDebugVariables.cpp

1640

Please can you update the comment since this function does now look through some non-debug instrs?

1661–1662

nit: It might be nice if there was a way to arrange this loop that avoids the it = std:next(it) requirement on every continue - I can't think of an easy way to do it though.

[Orlando]

I just wanted to add, this change makes the quadratic case mentioned by @jmorse in D105025 more likely since the function is able to scan back further in some cases. If your performance testing shows that these patches noticeably impact compile time - as an alternative to the "already inserted DBG_VALIE cache map" solution with this patch in mind - it might be worth trying the approach of RemoveRedundantDbgInstrs (added in D71478)? i.e. Let LiveDebugVariables insert all the DBG_VALUES it usually does and then run a cleanup afterwards that only needs to traverse the instructions twice.

[djtodoro]

@Orlando Thanks for your comments! Hmm, yes.. I've seen this patch, and I think it is a good approach, but I wanted to avoid introducing a new LLVM pass -- but as you pointed out, why not, it should be acceptable with the respect to time.

[djtodoro]

Alternative implemented as D105279.

llvm/lib/CodeGen/LiveDebugVariables.cpp

if (any_of(Regs, [&I, &TRI](Register &Reg) {
       // The insert location is directly after the instruction/bundle.
       return std::next(I);
     ++I;
   }
   return MBB->end();
 }
 
 // Avoid duplicates by scanning the machine basic block backward, up to the
 // first non-debug instr or up to the MBBStart.
 static bool shouldSkipThisDbgValue(MachineBasicBlock *MBB, MachineInstr &MI,
                                    const DILocalVariable *Variable,
                                    const MachineOperand &LocOp,
                                    const DIExpression *Expr, DebugLoc &dl) {
+  if (!LocOp.isReg())
+    return false;
+
   MachineBasicBlock::reverse_iterator it =
       std::next(MachineBasicBlock::reverse_iterator(MI));
   while (it != MBB->rend()) {
-    if (!it->isDebugValue())
+    if (!it->isDebugValue()) {
+      if (it->isMetaInstruction()) {
+        it = std::next(it);
+        continue;
+      }
+
+      const auto *TRI = MI.getMF()->getSubtarget().getRegisterInfo();
+      if (it->modifiesRegister(LocOp.getReg(), TRI))
         return false;
 
+      it = std::next(it);
+      continue;
+    }
+
     MachineOperand &loc = it->getDebugOperand(0);
-    if (Variable == it->getDebugVariable() && loc.isReg() && LocOp.isReg() &&
+    if (Variable == it->getDebugVariable() && loc.isReg() &&
         loc.getReg() == LocOp.getReg() && Expr == it->getDebugExpression() &&
         dl == it->getDebugLoc()) {
       LLVM_DEBUG(llvm::dbgs() << "\navoiding duplicated "; it->dump(););
       return true;
     }
 
     it = std::next(it);
   }

llvm/test/CodeGen/PowerPC/non-debug-mi-search-frspxsrsp.ll

 ; CHECK-NEXT:          .loc    1 0 45
 ; CHECK-NEXT:          xxlxor 2, 2, 2
 ; CHECK-NEXT:          .loc    1 3 26 is_stmt 1
 ; CHECK-NEXT:          xxmrghd 0, 0, 2
 ; CHECK-NEXT:          xxmrghd 1, 2, 1
 ; CHECK-NEXT:          xvcvdpsp 34, 0
 ; CHECK-NEXT:          xvcvdpsp 35, 1
 ; CHECK-NEXT:          vmrgew 2, 2, 3
-; CHECK-NEXT:          #DEBUG_VALUE: test:Vptr <- $x4
 ; CHECK-NEXT:          .loc    1 3 9 is_stmt 0
 ; CHECK-NEXT:          stvx 2, 0, 4
 ; CHECK-NEXT:          .loc    1 4 1 is_stmt 1
 ; CHECK-NEXT:          blr
 entry:
   call void @llvm.dbg.value(metadata float* %Fptr, metadata !19, metadata !DIExpression()), !dbg !22
   call void @llvm.dbg.value(metadata <4 x float>* %Vptr, metadata !20, metadata !DIExpression()), !dbg !22
   %0 = load float, float* %Fptr, align 4, !dbg !23, !tbaa !24