Add code to handle #debug_values in SROA.

[rastogishubham]

This patch adds a bunch of functions that will be used to properly handle debug information for #dbg_values in the SROA pass.

This is part of a stack of patches and is succeeded by: #94070

[llvmbot]

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Shubham Sandeep Rastogi (rastogishubham)

Changes

This patch adds a bunch of functions that will be used to properly handle debug information for llvm.dbg.values in the SROA pass.

---

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

6 Files Affected:

• (modified) llvm/include/llvm/IR/DebugInfo.h (+2)
• (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+5)
• (modified) llvm/include/llvm/IR/IntrinsicInst.h (+7)
• (modified) llvm/include/llvm/Transforms/Utils/Local.h (+7)
• (modified) llvm/lib/IR/DebugInfo.cpp (+17)
• (modified) llvm/lib/Transforms/Utils/Local.cpp (+28)

diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 5b80218d6c5cc..73f45c3769be4 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -43,6 +43,8 @@ class Module;
 TinyPtrVector<DbgDeclareInst *> findDbgDeclares(Value *V);
 /// As above, for DVRDeclares.
 TinyPtrVector<DbgVariableRecord *> findDVRDeclares(Value *V);
+/// As above, for DVRValues.
+TinyPtrVector<DbgVariableRecord *> findDVRValues(Value *V);
 
 /// Finds the llvm.dbg.value intrinsics describing a value.
 void findDbgValues(
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index ed8081a3cad19..72ff8f20d9006 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -426,6 +426,11 @@ class DbgVariableRecord : public DbgRecord, protected DebugValueUser {
   /// Does this describe the address of a local variable. True for dbg.addr
   /// and dbg.declare, but not dbg.value, which describes its value.
   bool isAddressOfVariable() const { return Type == LocationType::Declare; }
+
+  /// Does this describe the value of a local variable. False for dbg.addr
+  /// and dbg.declare, but not True dbg.value, which describes its value.
+  bool isValueOfVariable() const { return Type == LocationType::Value; }
+
   LocationType getType() const { return Type; }
 
   void setKillLocation();
diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index fcd3a1025ac13..7168a17212313 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -341,6 +341,13 @@ class DbgVariableIntrinsic : public DbgInfoIntrinsic {
     return getIntrinsicID() == Intrinsic::dbg_declare;
   }
 
+  /// Does this describe the value of a local variable. True for dbg.value,
+  /// but not dbg.declare, which describes its address, or dbg.assign, which
+  /// describes a combination of the variable's value and address.
+  bool isValueOfVariable() const {
+    return getIntrinsicID() == Intrinsic::dbg_value;
+  }
+
   void setKillLocation() {
     // TODO: When/if we remove duplicate values from DIArgLists, we don't need
     // this set anymore.
diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index 6937ec8dfd21c..d9ec9913eeb58 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -259,6 +259,13 @@ CallInst *changeToCall(InvokeInst *II, DomTreeUpdater *DTU = nullptr);
 ///  Dbg Intrinsic utilities
 ///
 
+/// Moves an llvm.dbg.value intrinsic before a store to an alloca'd value
+/// that has an associated llvm.dbg.value intrinsic.
+void MoveDebugValueToStoreLoc(DbgVariableRecord *DVR, StoreInst *SI,
+                              DIBuilder &Builder);
+void MoveDebugValueToStoreLoc(DbgVariableIntrinsic *DII, StoreInst *SI,
+                              DIBuilder &Builder);
+
 /// Inserts a llvm.dbg.value intrinsic before a store to an alloca'd value
 /// that has an associated llvm.dbg.declare intrinsic.
 void ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 4c3f37ceaaa46..dc3dad91db8f4 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -80,6 +80,23 @@ TinyPtrVector<DbgVariableRecord *> llvm::findDVRDeclares(Value *V) {
   return Declares;
 }
 
+TinyPtrVector<DbgVariableRecord *> llvm::findDVRValues(Value *V) {
+  // This function is hot. Check whether the value has any metadata to avoid a
+  // DenseMap lookup.
+  if (!V->isUsedByMetadata())
+    return {};
+  auto *L = LocalAsMetadata::getIfExists(V);
+  if (!L)
+    return {};
+
+  TinyPtrVector<DbgVariableRecord *> Values;
+  for (DbgVariableRecord *DVR : L->getAllDbgVariableRecordUsers())
+    if (DVR->getType() == DbgVariableRecord::LocationType::Value)
+      Values.push_back(DVR);
+
+  return Values;
+}
+
 template <typename IntrinsicT, bool DbgAssignAndValuesOnly>
 static void
 findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V,
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index ce0f4c7668a40..f90ee060ba3e3 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1731,6 +1731,21 @@ void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
                                     SI->getIterator());
 }
 
+/// Moves an llvm.dbg.value intrinsic before a store to an alloca'd value
+/// that has an associated llvm.dbg.value intrinsic.
+void llvm::MoveDebugValueToStoreLoc(DbgVariableIntrinsic *DII, StoreInst *SI,
+                                    DIBuilder &Builder) {
+  auto *DIVar = DII->getVariable();
+  assert(DIVar && "Missing variable");
+  auto *DIExpr = DII->getExpression();
+  Value *DV = SI->getValueOperand();
+
+  DebugLoc NewLoc = getDebugValueLoc(DII);
+
+  insertDbgValueOrDbgVariableRecord(Builder, DV, DIVar, DIExpr, NewLoc,
+                                    SI->getIterator());
+}
+
 /// Inserts a llvm.dbg.value intrinsic before a load of an alloca'd value
 /// that has an associated llvm.dbg.declare intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
@@ -1805,6 +1820,19 @@ void llvm::ConvertDebugDeclareToDebugValue(DbgVariableRecord *DVR,
   SI->getParent()->insertDbgRecordBefore(NewDVR, SI->getIterator());
 }
 
+void llvm::MoveDebugValueToStoreLoc(DbgVariableRecord *DVR, StoreInst *SI,
+                                    DIBuilder &Builder) {
+  auto *DIVar = DVR->getVariable();
+  assert(DIVar && "Missing variable");
+  auto *DIExpr = DVR->getExpression();
+  Value *DV = SI->getValueOperand();
+
+  DebugLoc NewLoc = getDebugValueLoc(DVR);
+
+  insertDbgValueOrDbgVariableRecord(Builder, DV, DIVar, DIExpr, NewLoc,
+                                    SI->getIterator());
+}
+
 /// Inserts a llvm.dbg.value intrinsic after a phi that has an associated
 /// llvm.dbg.declare intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,

[adrian-prantl]

If you add this in a separate commit, you should probably add a unit test.

[adrian-prantl]

llvm.dbg.addr has been removed from LLVM and replaced with dbg.value(op_deref)

[SLTozer]

Apologies, just noticed that the comment about adding a unit test hasn't been addressed yet. @adrian-prantl I think we don't have any unit tests that cover the existing parallels for the functions added in this commit, but I'd also accept a principled position against submitting code with no test coverage - do you think the lack-of a unit test is a blocker for this commit?

[rastogishubham]

@SLTozer @adrian-prantl since the existing parallels for the functions added in this commit do not have any unit tests, I don't think this code needs it either, though I agree with the sentiment of having unit tests for code that exists in a separate commit. I only separated out this code to make it easier to review. Since LLVM has moved to github, there is no correct way of doing stacked patches, if it is alright with the both of you, the PR that succeeds this PR (#94070) is the only one I would like to upstream, and I can squash the two commits into one when that patch is approved. Does that work for everyone?