fix jump threading failing to update cloned dbg.values

[BenJMudd]

Summary

This is a patch to fix duplicated dbg.values in the JumpThreading pass not pointing towards their local value, and instead towards the variable in the original block.
JumpThreadingPass::cloneInstructions is the changed function to target metadata as well as normal cloned values.

[probinson]

Style comments.

llvm/lib/Transforms/Scalar/JumpThreading.cpp

2053

Please don't mix unrelated formatting/whitespace changes with a functional change.

2087

2097

This one is debatable, if you prefer to keep it as is that's okay.

2101

Indentation here doesn't look right; try using clang-format-patch?

2144

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

7

[BenJMudd]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

2097

I think its more readable with auto, will change

[BenJMudd]

Addressing comments from Paul Robinson

[jmorse]

According to the style guide, "Variable names should [...] be camel case, and start with an upper case letter (e.g. Leader or Boats).", you'll need to update the variable names you've added. Otherwise all looks good.

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

5–6

I'd suggest rewording thus, to describe the action of transforming the IR, rather than the state at the end.

[BenJMudd]

Adding missed fullstop

[BenJMudd]

Changed variable names to camel case, and reworded the comment describing the test

[jmorse]

LGTM -- let's leave it a while to give more opportunity for input.

[Orlando]

This looks good but there's an edge case to consider:

dbg.value intrinsics can be associated with more than one value. I don't think this patch would update %b if it had an entry in ValueMapping for the intrinsic:

call void @llvm.dbg.value(metadata !DIArgList(i32 %a, i32 %b), metadata !n, metadata !DIExpression(DW_OP_arg, 0, DW_OP_arg, 1, DW_OP_plus, DW_OP_stack_value)))

You can iterate over the values using DbgVariableIntrinsic::location_ops() and you can replace them using DbgVariableIntrinsic::replaceVariableLocationOp.

It may be acceptable to leave that as a "TODO" item - I'll leave that up to @jmorse. If it's decided to leave the patch in its current form then please take a look at the inline comment I've added.

Thanks!

llvm/lib/Transforms/Scalar/JumpThreading.cpp

2099

IMO we should use the higher-level DbgVariableIntrinsic::replaceVariableLocationOp(unsigned OpIdx, Value *NewValue) here to replace the value rather than the more general/low-level method setOperand.

[jmorse]

In D140006#3995119, @Orlando wrote:

You can iterate over the values using DbgVariableIntrinsic::location_ops() and you can replace them using DbgVariableIntrinsic::replaceVariableLocationOp.

It may be acceptable to leave that as a "TODO" item - I'll leave that up to @jmorse. If it's decided to leave the patch in its current form then please take a look at the inline comment I've added.

Ah yeah, this is a good idea, and we shouldn't set the example that DIArgList dbg.values are ignorable for some reason.

[BenJMudd]

Refactor of test and to main lambda via Orlando's comment, this can now take into account !DIArgList

[StephenTozer]

Some notes about the DIArgList stuff, otherwise LGTM.

llvm/lib/Transforms/Scalar/JumpThreading.cpp

2100

Unfortunately the use of replaceVariableLocationOp here might invalidate the above iterator - specifically after calling this, DbgInstruction will have a new DIArgList operand but the iterator in this loop will be pointing to the old DIArgList. This will cause an error if an instruction to be replaced appears multiple times in the debug value: If you're replacing %0 with %1 in DIArgList(%0, %0), then in the first iteration the arglist will be updated to !DIArgList(%1, %1), then in the second iteration it will try to replace %0 again, which would trigger an assertion.

A fix for this would be to either build a list of values to be replaced inside the loop and then updating DbgInstruction outside of the loop, or iterating over indices in the loop and using the index version of replaceVariableLocationOp (personally I prefer the former, but either works).

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

30

As long as the test passes this should be fine, but worth noting some functions will assert that the DIExpression for a debug value with a DIArgList contains operands that reference those arguments; a simple example that would work here would be !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus).

[BenJMudd]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

2100

Ah I hadn't thought of the !DIArgList referencing the same value, it is definitely much safer to change the list outside of the loop. I also prefer the former, thanks

[BenJMudd]

Added support for !DIArgList containing duplicate values to be correctly remapped

[jmorse]

LGTM

llvm/lib/Transforms/Scalar/JumpThreading.cpp

for (Instruction &I : *BB) {
     // block, and if so, record them in UsesToRename.
     for (Use &U : I.uses()) {
       Instruction *User = cast<Instruction>(U.getUser());
       if (PHINode *UserPN = dyn_cast<PHINode>(User)) {
         if (UserPN->getIncomingBlock(U) == BB)
           continue;
       } else if (User->getParent() == BB)
         continue;
 
       UsesToRename.push_back(&U);
     }
 
     // If there are no uses outside the block, we're done with this instruction.
     if (UsesToRename.empty())
       continue;
     LLVM_DEBUG(dbgs() << "JT: Renaming non-local uses of: " << I << "\n");
 

 JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
                                      BasicBlock::iterator BE, BasicBlock *NewBB,
                                      BasicBlock *PredBB) {
   // We are going to have to map operands from the source basic block to the new
   // copy of the block 'NewBB'.  If there are PHI nodes in the source basic
   // block, evaluate them to account for entry from PredBB.
   DenseMap<Instruction *, Value *> ValueMapping;
 
+  // Retargets llvm.dbg.value to any renamed variables
+  auto retargetDbgValueIfPossible = [&](Instruction *NewInst) -> bool {
+    auto dbgInstruction = dyn_cast<DbgValueInst>(NewInst);
+    if (!dbgInstruction)
+      return false;
+
+    auto dbgValue = dyn_cast<Instruction>(dbgInstruction->getValue());
+    if (!dbgValue)
+      return false;
+
+    auto I = ValueMapping.find(dbgValue);
+    if (I != ValueMapping.end()) {
+      NewInst->setOperand(
+          0, MetadataAsValue::get(dbgInstruction->getContext(),
+                                  ValueAsMetadata::get(I->second)));
+      return true;
+    }
+    return false;
+  };
+
   // 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.
   for (; PHINode *PN = dyn_cast<PHINode>(BI); ++BI) {
     PHINode *NewPN = PHINode::Create(PN->getType(), 1, PN->getName(), NewBB);
     NewPN->addIncoming(PN->getIncomingValueForBlock(PredBB), PredBB);
     ValueMapping[PN] = NewPN;
   }
JumpThreadingPass::cloneInstructions(BasicBlock::iterator BI,
   // instructions.
   for (; BI != BE; ++BI) {
     Instruction *New = BI->clone();
     New->setName(BI->getName());
     NewBB->getInstList().push_back(New);
     ValueMapping[&*BI] = New;
     adaptNoAliasScopes(New, ClonedScopes, Context);
 
+    if (retargetDbgValueIfPossible(New))
+      continue;
+
     // Remap operands to patch up intra-block references.
     for (unsigned i = 0, e = New->getNumOperands(); i != e; ++i)
       if (Instruction *Inst = dyn_cast<Instruction>(New->getOperand(i))) {
         DenseMap<Instruction *, Value *>::iterator I = ValueMapping.find(Inst);
         if (I != ValueMapping.end())
           New->setOperand(i, I->second);
       }
   }
 
   return ValueMapping;
 }
 
 /// Attempt to thread through two successive basic blocks.
 bool JumpThreadingPass::maybethreadThroughTwoBasicBlocks(BasicBlock *BB,
                                                          Value *Cond) {

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

+; RUN: opt -S -passes=debugify,jump-threading < %s | FileCheck %s
+
+@a = global i32 0, align 4
+
+; Test that the llvm.dbg.value calls in a threaded block are correctly targeting
+; the locals in their block, and not the unthreaded one.
+define void @test2(i32 %cond1, i32 %cond2) {
+; CHECK: [[globalptr:@.*]] = global i32 0, align 4
+; CHECK: bb.cond2:
+; CHECK: call void @llvm.dbg.value(metadata ptr null, metadata ![[DBG1ptr:[0-9]+]], metadata !DIExpression()), !dbg ![[DBG2ptr:[0-9]+]]
+; CHECK-NEXT: [[TOBOOL1:%.*]] = icmp eq i32 %cond2, 0, !dbg ![[DBGLOCtobool1:[0-9]+]]
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i1 [[TOBOOL1]], metadata ![[DBG1tobool1:[0-9]+]], metadata !DIExpression()), !dbg ![[DBGLOCtobool1]]
+; CHECK: bb.cond2.thread:
+; CHECK-NEXT: call void @llvm.dbg.value(metadata ptr [[globalptr]], metadata ![[DBG1ptr]], metadata !DIExpression()), !dbg ![[DBG2ptr]]
+; CHECK-NEXT: [[TOBOOL12:%.*]] = icmp eq i32 %cond2, 0, !dbg ![[DBGLOCtobool1]]
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i1 [[TOBOOL12]], metadata ![[DBG1tobool1]], metadata !DIExpression()), !dbg ![[DBGLOCtobool1]]
+entry:
+  %tobool = icmp eq i32 %cond1, 0
+  br i1 %tobool, label %bb.cond2, label %bb.f1
+
+bb.f1:
+  call void @f1()
+  br label %bb.cond2
+
+bb.cond2:
+  %ptr = phi ptr [ null, %bb.f1 ], [ @a, %entry ]
+  %tobool1 = icmp eq i32 %cond2, 0
+  br i1 %tobool1, label %bb.file, label %bb.f2
+
+bb.f2:
+  call void @f2()
+  br label %exit
+
+bb.file:
+  %cmp = icmp eq ptr %ptr, null
+  br i1 %cmp, label %bb.f4, label %bb.f3
+
+bb.f3:
+  br label %exit
+
+bb.f4:
+  call void @f4()
+  br label %exit
+
+exit:
+  ret void
+}
+
+declare void @f1()
+
+declare void @f2()
+
+declare void @f3()
+
+declare void @f4()
 No newline at end of file