[Transforms] Debug values are not remapped when cloning. #87747

CarlosAlbertoEnciso · 2024-04-05T06:41:40Z

When cloning instructions from one basic block to another,
the debug values are not remapped, in the same was as the
normal instructions.

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions.

llvmbot · 2024-04-05T06:42:01Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Carlos Alberto Enciso (CarlosAlbertoEnciso)

Changes

When cloning instructions from one basic block to another,
the debug values are not remapped, in the same was as the
normal instructions.

Full diff: https://github.com/llvm/llvm-project/pull/87747.diff

5 Files Affected:

(modified) llvm/include/llvm/IR/IntrinsicInst.h (+2-1)
(modified) llvm/lib/IR/IntrinsicInst.cpp (+4-1)
(modified) llvm/lib/Transforms/Scalar/JumpThreading.cpp (+29)
(modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (+29)
(modified) llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll (+35-1)

diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index 4f22720f1c558d..e74bf15a21b9a2 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -311,7 +311,8 @@ class DbgVariableIntrinsic : public DbgInfoIntrinsic {
 
   Value *getVariableLocationOp(unsigned OpIdx) const;
 
-  void replaceVariableLocationOp(Value *OldValue, Value *NewValue);
+  void replaceVariableLocationOp(Value *OldValue, Value *NewValue,
+                                 bool AllowEmpty = false);
   void replaceVariableLocationOp(unsigned OpIdx, Value *NewValue);
   /// Adding a new location operand will always result in this intrinsic using
   /// an ArgList, and must always be accompanied by a new expression that uses
diff --git a/llvm/lib/IR/IntrinsicInst.cpp b/llvm/lib/IR/IntrinsicInst.cpp
index 89403e1d7fcb4d..cff716a8430992 100644
--- a/llvm/lib/IR/IntrinsicInst.cpp
+++ b/llvm/lib/IR/IntrinsicInst.cpp
@@ -119,7 +119,8 @@ static ValueAsMetadata *getAsMetadata(Value *V) {
 }
 
 void DbgVariableIntrinsic::replaceVariableLocationOp(Value *OldValue,
-                                                     Value *NewValue) {
+                                                     Value *NewValue,
+                                                     bool AllowEmpty) {
   // If OldValue is used as the address part of a dbg.assign intrinsic replace
   // it with NewValue and return true.
   auto ReplaceDbgAssignAddress = [this, OldValue, NewValue]() -> bool {
@@ -136,6 +137,8 @@ void DbgVariableIntrinsic::replaceVariableLocationOp(Value *OldValue,
   auto Locations = location_ops();
   auto OldIt = find(Locations, OldValue);
   if (OldIt == Locations.end()) {
+    if (AllowEmpty)
+      return;
     assert(DbgAssignAddrReplaced &&
            "OldValue must be dbg.assign addr if unused in DIArgList");
     return;
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index ffcb511e6a8312..d7936969011e66 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -2694,6 +2694,35 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
           New->setOperand(i, I->second);
       }
 
+    // Remap debug variable operands.
+    auto RemapDebugVariable = [](auto &Mapping, auto *Inst) {
+      auto RemapDebugOperands = [&Mapping](auto *DV, auto Set) {
+        for (auto *Op : Set)
+          if (Instruction *InstOp = dyn_cast<Instruction>(Op)) {
+            auto I = Mapping.find(InstOp);
+            if (I != Mapping.end())
+              DV->replaceVariableLocationOp(Op, I->second, /*AllowEmpty=*/true);
+          }
+      };
+      auto RemapAssignAddress = [&Mapping](auto *DA) {
+        if (Instruction *Addr = dyn_cast<Instruction>(DA->getAddress())) {
+          auto I = Mapping.find(Addr);
+          if (I != Mapping.end())
+            DA->setAddress(I->second);
+        }
+      };
+      if (auto DVI = dyn_cast<DbgVariableIntrinsic>(Inst))
+        RemapDebugOperands(DVI, DVI->location_ops());
+      if (auto DAI = dyn_cast<DbgAssignIntrinsic>(Inst))
+        RemapAssignAddress(DAI);
+      for (DbgVariableRecord &DVR : filterDbgVars(Inst->getDbgRecordRange())) {
+        RemapDebugOperands(&DVR, DVR.location_ops());
+        if (DVR.isDbgAssign())
+          RemapAssignAddress(&DVR);
+      }
+    };
+    RemapDebugVariable(ValueMapping, New);
+
     // If this instruction can be simplified after the operands are updated,
     // just use the simplified value instead.  This frequently happens due to
     // phi translation.
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 3eac726994ae13..de58e1b759f96b 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -1131,6 +1131,35 @@ BasicBlock *llvm::DuplicateInstructionsInSplitBetween(
         if (I != ValueMapping.end())
           New->setOperand(i, I->second);
       }
+
+    // Remap debug variable operands.
+    auto RemapDebugVariable = [](auto &Mapping, auto *Inst) {
+      auto RemapDebugOperands = [&Mapping](auto *DV, auto Set) {
+        for (auto *Op : Set)
+          if (Instruction *InstOp = dyn_cast<Instruction>(Op)) {
+            auto I = Mapping.find(InstOp);
+            if (I != Mapping.end())
+              DV->replaceVariableLocationOp(Op, I->second, /*AllowEmpty=*/true);
+          }
+      };
+      auto RemapAssignAddress = [&Mapping](auto *DA) {
+        if (Instruction *Addr = dyn_cast<Instruction>(DA->getAddress())) {
+          auto I = Mapping.find(Addr);
+          if (I != Mapping.end())
+            DA->setAddress(I->second);
+        }
+      };
+      if (auto DVI = dyn_cast<DbgVariableIntrinsic>(Inst))
+        RemapDebugOperands(DVI, DVI->location_ops());
+      if (auto DAI = dyn_cast<DbgAssignIntrinsic>(Inst))
+        RemapAssignAddress(DAI);
+      for (DbgVariableRecord &DVR : filterDbgVars(Inst->getDbgRecordRange())) {
+        RemapDebugOperands(&DVR, DVR.location_ops());
+        if (DVR.isDbgAssign())
+          RemapAssignAddress(&DVR);
+      }
+    };
+    RemapDebugVariable(ValueMapping, New);
   }
 
   return NewBB;
diff --git a/llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll b/llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll
index 8f10dcb30d7bfd..68c906d616c92d 100644
--- a/llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll
+++ b/llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -passes=callsite-splitting -o - < %s | FileCheck %s
+; RUN: opt -S -passes=callsite-splitting -o - < %s | FileCheck %s --check-prefixes=CHECK,CHECK-DEBUG
 ; RUN: opt -S -strip-debug -passes=callsite-splitting -o - < %s | FileCheck %s
 
 define internal i16 @bar(i16 %p1, i16 %p2) {
@@ -8,6 +8,9 @@ define internal i16 @bar(i16 %p1, i16 %p2) {
 
 define i16 @foo(i16 %in) {
 bb0:
+  %a = alloca i16, align 4, !DIAssignID !12
+  call void @llvm.dbg.assign(metadata i1 undef, metadata !11, metadata !DIExpression(), metadata !12, metadata ptr %a, metadata !DIExpression()), !dbg !8
+  store i16 7, ptr %a, align 4, !DIAssignID !13
   br label %bb1
 
 bb1:
@@ -20,13 +23,21 @@ bb2:
 CallsiteBB:
   %1 = phi i16 [ 0, %bb1 ], [ 1, %bb2 ]
   %c = phi i16 [ 2, %bb1 ], [ 3, %bb2 ]
+  %p = phi ptr [ %a, %bb1 ], [ %a, %bb2 ]
+  call void @llvm.dbg.value(metadata i16 %1, metadata !7, metadata !DIExpression()), !dbg !8
   call void @llvm.dbg.value(metadata i16 %c, metadata !7, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.value(metadata !DIArgList(i16 %1, i16 %c), metadata !7, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.value(metadata !DIArgList(i16 %c, i16 %c), metadata !7, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.assign(metadata i16 %1, metadata !11, metadata !DIExpression(), metadata !13, metadata ptr %a, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.assign(metadata i16 %c, metadata !11, metadata !DIExpression(), metadata !13, metadata ptr %a, metadata !DIExpression()), !dbg !8
+  call void @llvm.dbg.assign(metadata i16 %1, metadata !11, metadata !DIExpression(), metadata !13, metadata ptr %p, metadata !DIExpression()), !dbg !8
   %2 = call i16 @bar(i16 %1, i16 5)
   ret i16 %2
 }
 
 ; Function Attrs: nounwind readnone speculatable
 declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
 
 attributes #0 = { nounwind readnone speculatable }
 
@@ -43,14 +54,37 @@ attributes #0 = { nounwind readnone speculatable }
 !6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, unit: !0)
 !7 = !DILocalVariable(name: "c", scope: !6, line: 5, type: !5)
 !8 = !DILocation(line: 5, column: 7, scope: !6)
+!11 = !DILocalVariable(name: "a", scope: !6, line: 6, type: !5)
+!12 = distinct !DIAssignID()
+!13 = distinct !DIAssignID()
 
 ; The optimization should trigger even in the presence of the dbg.value in
 ; CallSiteBB.
 
 ; CHECK-LABEL: @foo
 ; CHECK-LABEL: bb1.split:
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata i16 0, metadata ![[DBG_1:[0-9]+]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata i16 2, metadata ![[DBG_1]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata !DIArgList(i16 0, i16 2), {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata !DIArgList(i16 2, i16 2), {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 0, metadata ![[DBG_2:[0-9]+]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 2, metadata ![[DBG_2]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 0, metadata ![[DBG_2]], metadata !DIExpression(), metadata ![[ID_1:[0-9]+]], metadata ptr %a, {{.*}}
 ; CHECK: [[TMP1:%[0-9]+]] = call i16 @bar(i16 0, i16 5)
+
 ; CHECK-LABEL: bb2.split:
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata i16 1, metadata ![[DBG_1]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata i16 3, metadata ![[DBG_1]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata !DIArgList(i16 1, i16 3), {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.value(metadata !DIArgList(i16 3, i16 3), {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 1, metadata ![[DBG_2]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 3, metadata ![[DBG_2]], {{.*}}
+; CHECK-DEBUG: call void @llvm.dbg.assign(metadata i16 1, metadata ![[DBG_2]], metadata !DIExpression(), metadata ![[ID_1:[0-9]+]], metadata ptr %a, {{.*}}
 ; CHECK: [[TMP2:%[0-9]+]] = call i16 @bar(i16 1, i16 5)
+
 ; CHECK-LABEL: CallsiteBB
 ; CHECK: %phi.call = phi i16 [ [[TMP2]], %bb2.split ], [ [[TMP1]], %bb1.split
+
+; CHECK-DEBUG-DAG: ![[DBG_1]] = !DILocalVariable(name: "c"{{.*}})
+; CHECK-DEBUG-DAG: ![[DBG_2]] = !DILocalVariable(name: "a"{{.*}})
+; CHECK-DEBUG-DAG: ![[ID_1]] = distinct !DIAssignID()

github-actions · 2024-04-05T06:43:02Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

OCHyams

Changes all look good, with one inline question.

OCHyams · 2024-04-08T05:37:32Z

llvm/lib/IR/IntrinsicInst.cpp

+    if (AllowEmpty)
+      return;


Why is this needed now (can you point to a test that fails without it)?

The included test fails:
llvm/test/Transforms/CallSiteSplitting/callsite-split-debug.ll

This is the line that triggers the assertion:
call void @llvm.dbg.value(metadata !DIArgList(i16 %c, i16 %c), metadata !7, metadata !DIExpression()), !dbg !8

Also, looking at the DbgVariableRecord::replaceVariableLocationOp the condition should be:

if (AllowEmpty || DbgAssignAddrReplaced) return;

Ah I see, yeah that makes sense. Thanks!

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. Missing condition '|| DbgAssignAddrReplaced' to account for any 'dbg.assign' replacement.

OCHyams

LGTM

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. - Make 'RemapDebugVariable' a general function by moving it to 'Transform/Utils/Local'. - Change in 'JumpThreading' parameters, return values and local instances: DenseMap<Instruction *, Value *> to ValueToValueMapTy.

CarlosAlbertoEnciso · 2024-04-16T13:03:42Z

@OCHyams Thanks for your review.
Moved the RemapDebugVariable code to Local as a general function.

OCHyams · 2024-04-16T13:07:48Z

llvm/lib/Transforms/Scalar/JumpThreading.cpp

+void JumpThreadingPass::cloneInstructions(ValueToValueMapTy &ValueMapping,
+                                          BasicBlock::iterator BI,
+                                          BasicBlock::iterator BE,
+                                          BasicBlock *NewBB,
+                                          BasicBlock *PredBB) {


Why is it necessary to change the return value to an out-parameter here?

Due to the compilation error: use of deleted function

// ValueMap can't be copied nor moved, because the callbacks store pointer to // it. ValueMap(const ValueMap &) = delete; ValueMap(ValueMap &&) = delete; ValueMap &operator=(const ValueMap &) = delete; ValueMap &operator=(ValueMap &&) = delete;

using ValueToValueMapTy = ValueMap<const Value *, WeakTrackingVH>;

ValueMap can't be copied nor moved.

OCHyams

Just two small things, then this LGTM. Thanks!

OCHyams · 2024-04-25T12:42:34Z

llvm/lib/Transforms/Utils/Local.cpp

+void llvm::remapDebugVariable(ValueToValueMapTy &Mapping, Instruction *Inst) {
+  auto RemapDebugOperands = [&Mapping](auto *DV, auto Set) {
+    for (auto *Op : Set)
+      if (Instruction *InstOp = dyn_cast<Instruction>(Op)) {


I don't think that we need the dyn_cast checks now that you're passing in a ValueToValueMapTy given that the key is a Value *. Could you remove them please?

OCHyams · 2024-04-25T12:49:05Z

llvm/include/llvm/Transforms/Utils/Local.h

+/// Remap the debug intrinsic instructions in the \p Mapping using the
+/// \p Inst as a criteria.


I think the wording of this comment is slightly unclear. What do you think of this instead?

"Remap the operands of the debug records attached to \p Inst, and the operands of \p Inst itself if it's a debug intrinsic."

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. - Rewording a comment to add more clarity. - Remove not required 'dyn_cast.

CarlosAlbertoEnciso · 2024-04-25T14:25:26Z

@OCHyams Thanks for your review.

OCHyams · 2024-04-25T14:39:08Z

llvm/lib/Transforms/Utils/Local.cpp

+void llvm::remapDebugVariable(ValueToValueMapTy &Mapping, Instruction *Inst) {
+  auto RemapDebugOperands = [&Mapping](auto *DV, auto Set) {
+    for (auto *Op : Set)
+      if (auto *InstOp = Op) {


You still have the if statements, but they're not needed either. Please can you remove those?

Same again below - I don't think if (Instruction *Addr = dyn_cast<Instruction>(DA->getAddress())) { is needed on line 3661.

@OCHyams Reworked the patch to remove those ifs. Thanks.

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. - After removing the 'dyn_cast' checks, the following 'if's are not required.

SLTozer

Comments look sufficiently addressed, LGTM

CarlosAlbertoEnciso · 2024-04-26T12:33:42Z

@SLTozer Thanks.

[Transforms] Debug values are not remapped when cloning.

2722cf8

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions.

CarlosAlbertoEnciso added debuginfo llvm:transforms labels Apr 5, 2024

CarlosAlbertoEnciso requested review from nikic, jryans, SLTozer, OCHyams and pogo59 April 5, 2024 06:41

CarlosAlbertoEnciso self-assigned this Apr 5, 2024

llvmbot added the llvm:ir label Apr 5, 2024

CarlosAlbertoEnciso requested review from jmorse and djtodoro April 5, 2024 07:09

OCHyams reviewed Apr 8, 2024

View reviewed changes

[Transforms] Debug values are not remapped when cloning.

21c102b

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. Missing condition '|| DbgAssignAddrReplaced' to account for any 'dbg.assign' replacement.

OCHyams approved these changes Apr 15, 2024

View reviewed changes

OCHyams reviewed Apr 16, 2024

View reviewed changes

OCHyams approved these changes Apr 25, 2024

View reviewed changes

[Transforms] Debug values are not remapped when cloning.

a935889

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. - Rewording a comment to add more clarity. - Remove not required 'dyn_cast.

OCHyams reviewed Apr 25, 2024

View reviewed changes

[Transforms] Debug values are not remapped when cloning.

bdcc5d5

When cloning instructions from one basic block to another, the debug values are not remapped, in the same was as the normal instructions. - After removing the 'dyn_cast' checks, the following 'if's are not required.

SLTozer approved these changes Apr 26, 2024

View reviewed changes

CarlosAlbertoEnciso merged commit 7696d36 into llvm:main Apr 26, 2024
3 of 4 checks passed

CarlosAlbertoEnciso deleted the remap-debug-values branch April 26, 2024 12:35

jthackray mentioned this pull request Apr 29, 2024

main #90439

Closed

jthackray mentioned this pull request Apr 29, 2024

[AArch64] Add support for Cortex-R82AE and improve Cortex-R82 #90440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transforms] Debug values are not remapped when cloning. #87747

[Transforms] Debug values are not remapped when cloning. #87747

CarlosAlbertoEnciso commented Apr 5, 2024

llvmbot commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024

OCHyams left a comment

OCHyams Apr 8, 2024

CarlosAlbertoEnciso Apr 9, 2024

CarlosAlbertoEnciso Apr 9, 2024 •

edited

Loading

OCHyams Apr 15, 2024

OCHyams left a comment

CarlosAlbertoEnciso commented Apr 16, 2024

OCHyams Apr 16, 2024

CarlosAlbertoEnciso Apr 17, 2024

OCHyams left a comment

OCHyams Apr 25, 2024

OCHyams Apr 25, 2024

CarlosAlbertoEnciso commented Apr 25, 2024

OCHyams Apr 25, 2024

OCHyams Apr 25, 2024

CarlosAlbertoEnciso Apr 26, 2024

SLTozer left a comment

CarlosAlbertoEnciso commented Apr 26, 2024

		/// Remap the debug intrinsic instructions in the \p Mapping using the
		/// \p Inst as a criteria.

[Transforms] Debug values are not remapped when cloning. #87747

[Transforms] Debug values are not remapped when cloning. #87747

Conversation

CarlosAlbertoEnciso commented Apr 5, 2024

llvmbot commented Apr 5, 2024 • edited Loading

github-actions bot commented Apr 5, 2024

OCHyams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OCHyams left a comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OCHyams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SLTozer left a comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Apr 26, 2024

llvmbot commented Apr 5, 2024 •

edited

Loading

CarlosAlbertoEnciso Apr 9, 2024 •

edited

Loading