[DebugInfo][RemoveDIs] Instrument jump-threading to update DPValues (#73127)

This patch makes jump-threading handle non-instruction debug-info stored
in DPValues in the same way that it updates dbg.values nowadays. This
involves re-targetting their operands as with dbg.values getting moved
from one block to another, and manually cloning them when duplicating
blocks. The SSAUpdater class also grows some functions for SSA-updating
DPValues in the same way as dbg.values.

All of this is largely covered by existing debug-info tests, except for
the cloning of DPValues attached to elidable instructions and branches,
where I've added a test to thread-debug-info.ll. Where previously we
could rely on dbg.values being copied and cloned as normal instructions
are, as we need to explicitly perform that operation now I've added some
explicit testing for it.

Author: jmorse

llvm/include/llvm/Transforms/Utils/SSAUpdater.h

@@ -23,6 +23,7 @@ class BasicBlock;
 class Instruction;
 class LoadInst;
 class PHINode;
+class DPValue;
 template <typename T> class SmallVectorImpl;
 template <typename T> class SSAUpdaterTraits;
 class Type;
@@ -123,6 +124,7 @@ class SSAUpdater {
   void UpdateDebugValues(Instruction *I);
   void UpdateDebugValues(Instruction *I,
                          SmallVectorImpl<DbgValueInst *> &DbgValues);
+  void UpdateDebugValues(Instruction *I, SmallVectorImpl<DPValue *> &DbgValues);
 
   /// Rewrite a use like \c RewriteUse but handling in-block definitions.
   ///
@@ -134,6 +136,7 @@ class SSAUpdater {
 private:
   Value *GetValueAtEndOfBlockInternal(BasicBlock *BB);
   void UpdateDebugValue(Instruction *I, DbgValueInst *DbgValue);
+  void UpdateDebugValue(Instruction *I, DPValue *DbgValue);
 };
 
 /// Helper class for promoting a collection of loads and stores into SSA

llvm/lib/Transforms/Scalar/JumpThreading.cpp

@@ -410,6 +410,10 @@ static bool replaceFoldableUses(Instruction *Cond, Value *ToVal,
   if (Cond->getParent() == KnownAtEndOfBB)
     Changed |= replaceNonLocalUsesWith(Cond, ToVal);
   for (Instruction &I : reverse(*KnownAtEndOfBB)) {
+    // Replace any debug-info record users of Cond with ToVal.
+    for (DPValue &DPV : I.getDbgValueRange())
+      DPV.replaceVariableLocationOp(Cond, ToVal, true);
+
     // Reached the Cond whose uses we are trying to replace, so there are no
     // more uses.
     if (&I == Cond)
@@ -1961,6 +1965,7 @@ void JumpThreadingPass::updateSSA(
   SSAUpdater SSAUpdate;
   SmallVector<Use *, 16> UsesToRename;
   SmallVector<DbgValueInst *, 4> DbgValues;
+  SmallVector<DPValue *, 4> DPValues;
 
   for (Instruction &I : *BB) {
     // Scan all uses of this instruction to see if it is used outside of its
@@ -1977,10 +1982,13 @@ void JumpThreadingPass::updateSSA(
     }
 
     // Find debug values outside of the block
-    findDbgValues(DbgValues, &I);
+    findDbgValues(DbgValues, &I, &DPValues);
     llvm::erase_if(DbgValues, [&](const DbgValueInst *DbgVal) {
       return DbgVal->getParent() == BB;
     });
+    llvm::erase_if(DPValues, [&](const DPValue *DPVal) {
+      return DPVal->getParent() == BB;
+    });
 
     // If there are no uses outside the block, we're done with this instruction.
     if (UsesToRename.empty() && DbgValues.empty())
@@ -1996,9 +2004,11 @@ void JumpThreadingPass::updateSSA(
 
     while (!UsesToRename.empty())
       SSAUpdate.RewriteUse(*UsesToRename.pop_back_val());
-    if (!DbgValues.empty()) {
+    if (!DbgValues.empty() || !DPValues.empty()) {
       SSAUpdate.UpdateDebugValues(&I, DbgValues);
+      SSAUpdate.UpdateDebugValues(&I, DPValues);
       DbgValues.clear();
+      DPValues.clear();
     }
 
     LLVM_DEBUG(dbgs() << "\n");
@@ -2041,6 +2051,26 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
     return true;
   };
 
+  // Duplicate implementation of the above dbg.value code, using DPValues
+  // instead.
+  auto RetargetDPValueIfPossible = [&](DPValue *DPV) {
+    SmallSet<std::pair<Value *, Value *>, 16> OperandsToRemap;
+    for (auto *Op : DPV->location_ops()) {
+      Instruction *OpInst = dyn_cast<Instruction>(Op);
+      if (!OpInst)
+        continue;
+
+      auto I = ValueMapping.find(OpInst);
+      if (I != ValueMapping.end())
+        OperandsToRemap.insert({OpInst, I->second});
+    }
+
+    for (auto &[OldOp, MappedOp] : OperandsToRemap)
+      DPV->replaceVariableLocationOp(OldOp, MappedOp);
+  };
+
+  BasicBlock *RangeBB = BI->getParent();
+
   // Clone the phi nodes of the source basic block into NewBB.  The resulting
   // phi nodes are trivial since NewBB only has one predecessor, but SSAUpdater
   // might need to rewrite the operand of the cloned phi.
@@ -2059,6 +2089,12 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
   identifyNoAliasScopesToClone(BI, BE, NoAliasScopes);
   cloneNoAliasScopes(NoAliasScopes, ClonedScopes, "thread", Context);
 
+  auto CloneAndRemapDbgInfo = [&](Instruction *NewInst, Instruction *From) {
+    auto DPVRange = NewInst->cloneDebugInfoFrom(From);
+    for (DPValue &DPV : DPVRange)
+      RetargetDPValueIfPossible(&DPV);
+  };
+
   // Clone the non-phi instructions of the source basic block into NewBB,
   // keeping track of the mapping and using it to remap operands in the cloned
   // instructions.
@@ -2069,6 +2105,8 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
     ValueMapping[&*BI] = New;
     adaptNoAliasScopes(New, ClonedScopes, Context);
 
+    CloneAndRemapDbgInfo(New, &*BI);
+
     if (RetargetDbgValueIfPossible(New))
       continue;
 
@@ -2081,6 +2119,17 @@ JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
       }
   }
 
+  // There may be DPValues on the terminator, clone directly from marker
+  // to marker as there isn't an instruction there.
+  if (BE != RangeBB->end() && BE->hasDbgValues()) {
+    // Dump them at the end.
+    DPMarker *Marker = RangeBB->getMarker(BE);
+    DPMarker *EndMarker = NewBB->createMarker(NewBB->end());
+    auto DPVRange = EndMarker->cloneDebugInfoFrom(Marker, std::nullopt);
+    for (DPValue &DPV : DPVRange)
+      RetargetDPValueIfPossible(&DPV);
+  }
+
   return ValueMapping;
 }
 
@@ -2666,13 +2715,18 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
       if (!New->mayHaveSideEffects()) {
         New->eraseFromParent();
         New = nullptr;
+        // Clone debug-info on the elided instruction to the destination
+        // position.
+        OldPredBranch->cloneDebugInfoFrom(&*BI, std::nullopt, true);
       }
     } else {
       ValueMapping[&*BI] = New;
     }
     if (New) {
       // Otherwise, insert the new instruction into the block.
       New->setName(BI->getName());
+      // Clone across any debug-info attached to the old instruction.
+      New->cloneDebugInfoFrom(&*BI);
       // Update Dominance from simplified New instruction operands.
       for (unsigned i = 0, e = New->getNumOperands(); i != e; ++i)
         if (BasicBlock *SuccBB = dyn_cast<BasicBlock>(New->getOperand(i)))

llvm/lib/Transforms/Utils/SSAUpdater.cpp

@@ -199,12 +199,18 @@ void SSAUpdater::RewriteUse(Use &U) {
 
 void SSAUpdater::UpdateDebugValues(Instruction *I) {
   SmallVector<DbgValueInst *, 4> DbgValues;
-  llvm::findDbgValues(DbgValues, I);
+  SmallVector<DPValue *, 4> DPValues;
+  llvm::findDbgValues(DbgValues, I, &DPValues);
   for (auto &DbgValue : DbgValues) {
     if (DbgValue->getParent() == I->getParent())
       continue;
     UpdateDebugValue(I, DbgValue);
   }
+  for (auto &DPV : DPValues) {
+    if (DPV->getParent() == I->getParent())
+      continue;
+    UpdateDebugValue(I, DPV);
+  }
 }
 
 void SSAUpdater::UpdateDebugValues(Instruction *I,
@@ -214,16 +220,31 @@ void SSAUpdater::UpdateDebugValues(Instruction *I,
   }
 }
 
+void SSAUpdater::UpdateDebugValues(Instruction *I,
+                                   SmallVectorImpl<DPValue *> &DPValues) {
+  for (auto &DPV : DPValues) {
+    UpdateDebugValue(I, DPV);
+  }
+}
+
 void SSAUpdater::UpdateDebugValue(Instruction *I, DbgValueInst *DbgValue) {
   BasicBlock *UserBB = DbgValue->getParent();
   if (HasValueForBlock(UserBB)) {
     Value *NewVal = GetValueAtEndOfBlock(UserBB);
     DbgValue->replaceVariableLocationOp(I, NewVal);
-  }
-  else
+  } else
     DbgValue->setKillLocation();
 }
 
+void SSAUpdater::UpdateDebugValue(Instruction *I, DPValue *DPV) {
+  BasicBlock *UserBB = DPV->getParent();
+  if (HasValueForBlock(UserBB)) {
+    Value *NewVal = GetValueAtEndOfBlock(UserBB);
+    DPV->replaceVariableLocationOp(I, NewVal);
+  } else
+    DPV->setKillLocation();
+}
+
 void SSAUpdater::RewriteUseAfterInsertions(Use &U) {
   Instruction *User = cast<Instruction>(U.getUser());
 

llvm/test/Transforms/JumpThreading/redundant-dbg-info.ll

@@ -1,4 +1,5 @@
 ; RUN: opt -passes=jump-threading -S < %s | FileCheck %s
+; RUN: opt -passes=jump-threading -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
 
 define dso_local i32 @_Z3fooi(i32 %a) !dbg !7 {
 entry:

llvm/test/Transforms/JumpThreading/thread-debug-info.ll

@@ -1,4 +1,5 @@
 ; RUN: opt -S -passes=jump-threading < %s | FileCheck %s
+; RUN: opt -S -passes=jump-threading < %s --try-experimental-debuginfo-iterators | FileCheck %s
 
 @a = global i32 0, align 4
 ; Test that the llvm.dbg.value calls in a threaded block are correctly updated to
@@ -91,6 +92,47 @@ exit:                                             ; preds = %bb.f4, %bb.f3, %bb.
   ret void, !dbg !29
 }
 
+; Test for the cloning of dbg.values on elided instructions -- down one path
+; being threaded, the `and` in the function below is optimised away, but its
+; debug-info should still be preserved.
+; Similarly, the call to f1 gets cloned, its dbg.value should be cloned too.
+define void @test16(i1 %c, i1 %c2, i1 %c3, i1 %c4) nounwind ssp !dbg !30 {
+; CHECK-LABEL: define void @test16(i1
+entry:
+  %cmp = icmp sgt i32 undef, 1, !dbg !33
+  br i1 %c, label %land.end, label %land.rhs, !dbg !33
+
+land.rhs:
+  br i1 %c2, label %lor.lhs.false.i, label %land.end, !dbg !33
+
+lor.lhs.false.i:
+  br i1 %c3, label %land.end, label %land.end, !dbg !33
+
+; CHECK-LABEL: land.end.thr_comm:
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 0,
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 1,
+; CHECK-NEXT:  call void @f1()
+; CHECK-NEXT:  br i1 %c4,
+
+; CHECK-LABEL: land.end:
+; CHECK-NEXT:  %0 = phi i1
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 0,
+land.end:
+  %0 = phi i1 [ true, %entry ], [ false, %land.rhs ], [false, %lor.lhs.false.i], [false, %lor.lhs.false.i]
+  call void @llvm.dbg.value(metadata i32 0, metadata !32, metadata !DIExpression()), !dbg !33
+  %cmp12 = and i1 %cmp, %0, !dbg !33
+  %xor1 = xor i1 %cmp12, %c4, !dbg !33
+  call void @llvm.dbg.value(metadata i32 1, metadata !32, metadata !DIExpression()), !dbg !33
+  call void @f1()
+  br i1 %xor1, label %if.then, label %if.end, !dbg !33
+
+if.then:
+  ret void, !dbg !33
+
+if.end:
+  ret void, !dbg !33
+}
+
 declare void @f1()
 
 declare void @f2()
@@ -138,3 +180,7 @@ attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memo
 !27 = !DILocation(line: 13, column: 1, scope: !5)
 !28 = !DILocation(line: 14, column: 1, scope: !5)
 !29 = !DILocation(line: 15, column: 1, scope: !5)
+!30 = distinct !DISubprogram(name: "test13", linkageName: "test13", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !31)
+!31 = !{!32}
+!32 = !DILocalVariable(name: "1", scope: !30, file: !1, line: 1, type: !10)
+!33 = !DILocation(line: 1, column: 1, scope: !30)