Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RemoveDIs][NFC] Find DPValues using findDbgDeclares #73500

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Nov 27, 2023

This patch doesn't change any call sites.

Depends on #73498

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 27, 2023

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

This change doesn't change any call sites.


I'm not sure what the best way to do stacked reviews is, but this is meant to only be a review for the second commit.

Based on top of #73498


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

6 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugInfo.h (+2-1)
  • (modified) llvm/lib/IR/DebugInfo.cpp (+16-27)
  • (modified) llvm/lib/Transforms/Coroutines/CoroFrame.cpp (+8-4)
  • (modified) llvm/lib/Transforms/Scalar/SROA.cpp (+10-3)
  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+2-1)
  • (modified) llvm/lib/Transforms/Utils/MemoryOpRemark.cpp (+3-2)
diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 2a581eb5f09d9f8..36ef77f9505bc10 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -40,7 +40,8 @@ class Module;
 
 /// Finds dbg.declare intrinsics declaring local variables as living in the
 /// memory that 'V' points to.
-TinyPtrVector<DbgDeclareInst *> FindDbgDeclareUses(Value *V);
+void findDbgDeclares(SmallVectorImpl<DbgDeclareInst *> &DbgUsers, Value *V,
+                     SmallVectorImpl<DPValue *> *DPValues = nullptr);
 
 /// Finds the llvm.dbg.value intrinsics describing a value.
 void findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues,
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 3fe940e19295b01..98271f1a2a88a72 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -44,30 +44,12 @@ using namespace llvm;
 using namespace llvm::at;
 using namespace llvm::dwarf;
 
-TinyPtrVector<DbgDeclareInst *> llvm::FindDbgDeclareUses(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 {};
-  auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L);
-  if (!MDV)
-    return {};
-
-  TinyPtrVector<DbgDeclareInst *> Declares;
-  for (User *U : MDV->users()) {
-    if (auto *DDI = dyn_cast<DbgDeclareInst>(U))
-      Declares.push_back(DDI);
-  }
+template <typename IntrinsicT, bool AnyType,
+          DPValue::LocationType Type = DPValue::LocationType(-1)>
+static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V,
+                              SmallVectorImpl<DPValue *> *DPValues) {
+  static_assert(AnyType || (int)Type >= 0);
 
-  return Declares;
-}
-
-template <typename IntrinsicT>
-static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
-                              Value *V, SmallVectorImpl<DPValue *> *DPValues) {
   // This function is hot. Check whether the value has any metadata to avoid a
   // DenseMap lookup.
   if (!V->isUsedByMetadata())
@@ -96,7 +78,7 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
     // Get DPValues that use this as a single value.
     if (LocalAsMetadata *L = dyn_cast<LocalAsMetadata>(MD)) {
       for (DPValue *DPV : L->getAllDPValueUsers()) {
-        if (DPV->getType() == DPValue::LocationType::Value)
+        if (AnyType || DPV->getType() == Type)
           DPValues->push_back(DPV);
       }
     }
@@ -110,21 +92,28 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
         continue;
       DIArgList *DI = cast<DIArgList>(AL);
       for (DPValue *DPV : DI->getAllDPValueUsers())
-        if (DPV->getType() == DPValue::LocationType::Value)
+        if (AnyType || DPV->getType() == Type)
           if (EncounteredDPValues.insert(DPV).second)
             DPValues->push_back(DPV);
     }
   }
 }
 
+void llvm::findDbgDeclares(SmallVectorImpl<DbgDeclareInst *> &DbgUsers,
+                           Value *V, SmallVectorImpl<DPValue *> *DPValues) {
+  findDbgIntrinsics<DbgDeclareInst, false, DPValue::LocationType::Declare>(
+      DbgUsers, V, DPValues);
+}
+
 void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues,
                          Value *V, SmallVectorImpl<DPValue *> *DPValues) {
-  findDbgIntrinsics<DbgValueInst>(DbgValues, V, DPValues);
+  findDbgIntrinsics<DbgValueInst, false, DPValue::LocationType::Value>(
+      DbgValues, V, DPValues);
 }
 
 void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
                         Value *V, SmallVectorImpl<DPValue *> *DPValues) {
-  findDbgIntrinsics<DbgVariableIntrinsic>(DbgUsers, V, DPValues);
+  findDbgIntrinsics<DbgVariableIntrinsic, true>(DbgUsers, V, DPValues);
 }
 
 DISubprogram *llvm::getDISubprogram(const MDNode *Scope) {
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index fef1a698e146390..fbacfb39f62acbd 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -963,7 +963,8 @@ static void cacheDIVar(FrameDataInfo &FrameData,
     if (DIVarCache.contains(V))
       continue;
 
-    auto DDIs = FindDbgDeclareUses(V);
+    SmallVector<DbgDeclareInst *> DDIs;
+    findDbgDeclares(DDIs, V);
     auto *I = llvm::find_if(DDIs, [](DbgDeclareInst *DDI) {
       return DDI->getExpression()->getNumElements() == 0;
     });
@@ -1119,7 +1120,8 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
   assert(PromiseAlloca &&
          "Coroutine with switch ABI should own Promise alloca");
 
-  TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(PromiseAlloca);
+  SmallVector<DbgDeclareInst *> DIs;
+  findDbgDeclares(DIs, PromiseAlloca);
   if (DIs.empty())
     return;
 
@@ -1840,7 +1842,8 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
               FrameTy->getElementType(FrameData.getFieldIndex(E.first)), GEP,
               SpillAlignment, E.first->getName() + Twine(".reload"));
 
-        TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Def);
+        SmallVector<DbgDeclareInst *> DIs;
+        findDbgDeclares(DIs, Def);
         // Try best to find dbg.declare. If the spill is a temp, there may not
         // be a direct dbg.declare. Walk up the load chain to find one from an
         // alias.
@@ -1854,7 +1857,8 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
             CurDef = LdInst->getPointerOperand();
             if (!isa<AllocaInst, LoadInst>(CurDef))
               break;
-            DIs = FindDbgDeclareUses(CurDef);
+            DIs.clear();
+            findDbgDeclares(DIs, CurDef);
           }
         }
 
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index f578762d2b4971c..92d5f970ea4ccab 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -4940,10 +4940,13 @@ bool SROA::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
   // Migrate debug information from the old alloca to the new alloca(s)
   // and the individual partitions.
   TinyPtrVector<DbgVariableIntrinsic *> DbgVariables;
-  for (auto *DbgDeclare : FindDbgDeclareUses(&AI))
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, &AI);
+  for (auto *DbgDeclare : DbgDeclares)
     DbgVariables.push_back(DbgDeclare);
   for (auto *DbgAssign : at::getAssignmentMarkers(&AI))
     DbgVariables.push_back(DbgAssign);
+
   for (DbgVariableIntrinsic *DbgVariable : DbgVariables) {
     auto *Expr = DbgVariable->getExpression();
     DIBuilder DIB(*AI.getModule(), /*AllowUnresolved*/ false);
@@ -4997,7 +5000,9 @@ bool SROA::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
 
       // Remove any existing intrinsics on the new alloca describing
       // the variable fragment.
-      for (DbgDeclareInst *OldDII : FindDbgDeclareUses(Fragment.Alloca)) {
+      SmallVector<DbgDeclareInst *> FragDbgDeclares;
+      findDbgDeclares(FragDbgDeclares, Fragment.Alloca);
+      for (DbgDeclareInst *OldDII : FragDbgDeclares) {
         auto SameVariableFragment = [](const DbgVariableIntrinsic *LHS,
                                        const DbgVariableIntrinsic *RHS) {
           return LHS->getVariable() == RHS->getVariable() &&
@@ -5147,7 +5152,9 @@ bool SROA::deleteDeadInstructions(
     // not be able to find it.
     if (AllocaInst *AI = dyn_cast<AllocaInst>(I)) {
       DeletedAllocas.insert(AI);
-      for (DbgDeclareInst *OldDII : FindDbgDeclareUses(AI))
+      SmallVector<DbgDeclareInst *> DbgDeclares;
+      findDbgDeclares(DbgDeclares, AI);
+      for (DbgDeclareInst *OldDII : DbgDeclares)
         OldDII->eraseFromParent();
     }
 
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index e399329a58873e7..4ef5153e20838ed 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -2103,7 +2103,8 @@ void llvm::insertDebugValuesForPHIs(BasicBlock *BB,
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
                              DIBuilder &Builder, uint8_t DIExprFlags,
                              int Offset) {
-  auto DbgDeclares = FindDbgDeclareUses(Address);
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, Address);
   for (DbgVariableIntrinsic *DII : DbgDeclares) {
     const DebugLoc &Loc = DII->getDebugLoc();
     auto *DIVar = DII->getVariable();
diff --git a/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp b/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
index 531b0a624dafab6..99c033cab64f803 100644
--- a/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
+++ b/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
@@ -321,8 +321,9 @@ void MemoryOpRemark::visitVariable(const Value *V,
   bool FoundDI = false;
   // Try to get an llvm.dbg.declare, which has a DILocalVariable giving us the
   // real debug info name and size of the variable.
-  for (const DbgVariableIntrinsic *DVI :
-       FindDbgDeclareUses(const_cast<Value *>(V))) {
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, const_cast<Value *>(V));
+  for (const DbgVariableIntrinsic *DVI : DbgDeclares) {
     if (DILocalVariable *DILV = DVI->getVariable()) {
       std::optional<uint64_t> DISize = getSizeInBytes(DILV->getSizeInBits());
       VariableInfo Var{DILV->getName(), DISize};

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 27, 2023

@llvm/pr-subscribers-llvm-ir

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

This change doesn't change any call sites.


I'm not sure what the best way to do stacked reviews is, but this is meant to only be a review for the second commit.

Based on top of #73498


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

6 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugInfo.h (+2-1)
  • (modified) llvm/lib/IR/DebugInfo.cpp (+16-27)
  • (modified) llvm/lib/Transforms/Coroutines/CoroFrame.cpp (+8-4)
  • (modified) llvm/lib/Transforms/Scalar/SROA.cpp (+10-3)
  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+2-1)
  • (modified) llvm/lib/Transforms/Utils/MemoryOpRemark.cpp (+3-2)
diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 2a581eb5f09d9f8..36ef77f9505bc10 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -40,7 +40,8 @@ class Module;
 
 /// Finds dbg.declare intrinsics declaring local variables as living in the
 /// memory that 'V' points to.
-TinyPtrVector<DbgDeclareInst *> FindDbgDeclareUses(Value *V);
+void findDbgDeclares(SmallVectorImpl<DbgDeclareInst *> &DbgUsers, Value *V,
+                     SmallVectorImpl<DPValue *> *DPValues = nullptr);
 
 /// Finds the llvm.dbg.value intrinsics describing a value.
 void findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues,
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 3fe940e19295b01..98271f1a2a88a72 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -44,30 +44,12 @@ using namespace llvm;
 using namespace llvm::at;
 using namespace llvm::dwarf;
 
-TinyPtrVector<DbgDeclareInst *> llvm::FindDbgDeclareUses(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 {};
-  auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L);
-  if (!MDV)
-    return {};
-
-  TinyPtrVector<DbgDeclareInst *> Declares;
-  for (User *U : MDV->users()) {
-    if (auto *DDI = dyn_cast<DbgDeclareInst>(U))
-      Declares.push_back(DDI);
-  }
+template <typename IntrinsicT, bool AnyType,
+          DPValue::LocationType Type = DPValue::LocationType(-1)>
+static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V,
+                              SmallVectorImpl<DPValue *> *DPValues) {
+  static_assert(AnyType || (int)Type >= 0);
 
-  return Declares;
-}
-
-template <typename IntrinsicT>
-static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
-                              Value *V, SmallVectorImpl<DPValue *> *DPValues) {
   // This function is hot. Check whether the value has any metadata to avoid a
   // DenseMap lookup.
   if (!V->isUsedByMetadata())
@@ -96,7 +78,7 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
     // Get DPValues that use this as a single value.
     if (LocalAsMetadata *L = dyn_cast<LocalAsMetadata>(MD)) {
       for (DPValue *DPV : L->getAllDPValueUsers()) {
-        if (DPV->getType() == DPValue::LocationType::Value)
+        if (AnyType || DPV->getType() == Type)
           DPValues->push_back(DPV);
       }
     }
@@ -110,21 +92,28 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result,
         continue;
       DIArgList *DI = cast<DIArgList>(AL);
       for (DPValue *DPV : DI->getAllDPValueUsers())
-        if (DPV->getType() == DPValue::LocationType::Value)
+        if (AnyType || DPV->getType() == Type)
           if (EncounteredDPValues.insert(DPV).second)
             DPValues->push_back(DPV);
     }
   }
 }
 
+void llvm::findDbgDeclares(SmallVectorImpl<DbgDeclareInst *> &DbgUsers,
+                           Value *V, SmallVectorImpl<DPValue *> *DPValues) {
+  findDbgIntrinsics<DbgDeclareInst, false, DPValue::LocationType::Declare>(
+      DbgUsers, V, DPValues);
+}
+
 void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues,
                          Value *V, SmallVectorImpl<DPValue *> *DPValues) {
-  findDbgIntrinsics<DbgValueInst>(DbgValues, V, DPValues);
+  findDbgIntrinsics<DbgValueInst, false, DPValue::LocationType::Value>(
+      DbgValues, V, DPValues);
 }
 
 void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
                         Value *V, SmallVectorImpl<DPValue *> *DPValues) {
-  findDbgIntrinsics<DbgVariableIntrinsic>(DbgUsers, V, DPValues);
+  findDbgIntrinsics<DbgVariableIntrinsic, true>(DbgUsers, V, DPValues);
 }
 
 DISubprogram *llvm::getDISubprogram(const MDNode *Scope) {
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index fef1a698e146390..fbacfb39f62acbd 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -963,7 +963,8 @@ static void cacheDIVar(FrameDataInfo &FrameData,
     if (DIVarCache.contains(V))
       continue;
 
-    auto DDIs = FindDbgDeclareUses(V);
+    SmallVector<DbgDeclareInst *> DDIs;
+    findDbgDeclares(DDIs, V);
     auto *I = llvm::find_if(DDIs, [](DbgDeclareInst *DDI) {
       return DDI->getExpression()->getNumElements() == 0;
     });
@@ -1119,7 +1120,8 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
   assert(PromiseAlloca &&
          "Coroutine with switch ABI should own Promise alloca");
 
-  TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(PromiseAlloca);
+  SmallVector<DbgDeclareInst *> DIs;
+  findDbgDeclares(DIs, PromiseAlloca);
   if (DIs.empty())
     return;
 
@@ -1840,7 +1842,8 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
               FrameTy->getElementType(FrameData.getFieldIndex(E.first)), GEP,
               SpillAlignment, E.first->getName() + Twine(".reload"));
 
-        TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Def);
+        SmallVector<DbgDeclareInst *> DIs;
+        findDbgDeclares(DIs, Def);
         // Try best to find dbg.declare. If the spill is a temp, there may not
         // be a direct dbg.declare. Walk up the load chain to find one from an
         // alias.
@@ -1854,7 +1857,8 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
             CurDef = LdInst->getPointerOperand();
             if (!isa<AllocaInst, LoadInst>(CurDef))
               break;
-            DIs = FindDbgDeclareUses(CurDef);
+            DIs.clear();
+            findDbgDeclares(DIs, CurDef);
           }
         }
 
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index f578762d2b4971c..92d5f970ea4ccab 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -4940,10 +4940,13 @@ bool SROA::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
   // Migrate debug information from the old alloca to the new alloca(s)
   // and the individual partitions.
   TinyPtrVector<DbgVariableIntrinsic *> DbgVariables;
-  for (auto *DbgDeclare : FindDbgDeclareUses(&AI))
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, &AI);
+  for (auto *DbgDeclare : DbgDeclares)
     DbgVariables.push_back(DbgDeclare);
   for (auto *DbgAssign : at::getAssignmentMarkers(&AI))
     DbgVariables.push_back(DbgAssign);
+
   for (DbgVariableIntrinsic *DbgVariable : DbgVariables) {
     auto *Expr = DbgVariable->getExpression();
     DIBuilder DIB(*AI.getModule(), /*AllowUnresolved*/ false);
@@ -4997,7 +5000,9 @@ bool SROA::splitAlloca(AllocaInst &AI, AllocaSlices &AS) {
 
       // Remove any existing intrinsics on the new alloca describing
       // the variable fragment.
-      for (DbgDeclareInst *OldDII : FindDbgDeclareUses(Fragment.Alloca)) {
+      SmallVector<DbgDeclareInst *> FragDbgDeclares;
+      findDbgDeclares(FragDbgDeclares, Fragment.Alloca);
+      for (DbgDeclareInst *OldDII : FragDbgDeclares) {
         auto SameVariableFragment = [](const DbgVariableIntrinsic *LHS,
                                        const DbgVariableIntrinsic *RHS) {
           return LHS->getVariable() == RHS->getVariable() &&
@@ -5147,7 +5152,9 @@ bool SROA::deleteDeadInstructions(
     // not be able to find it.
     if (AllocaInst *AI = dyn_cast<AllocaInst>(I)) {
       DeletedAllocas.insert(AI);
-      for (DbgDeclareInst *OldDII : FindDbgDeclareUses(AI))
+      SmallVector<DbgDeclareInst *> DbgDeclares;
+      findDbgDeclares(DbgDeclares, AI);
+      for (DbgDeclareInst *OldDII : DbgDeclares)
         OldDII->eraseFromParent();
     }
 
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index e399329a58873e7..4ef5153e20838ed 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -2103,7 +2103,8 @@ void llvm::insertDebugValuesForPHIs(BasicBlock *BB,
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
                              DIBuilder &Builder, uint8_t DIExprFlags,
                              int Offset) {
-  auto DbgDeclares = FindDbgDeclareUses(Address);
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, Address);
   for (DbgVariableIntrinsic *DII : DbgDeclares) {
     const DebugLoc &Loc = DII->getDebugLoc();
     auto *DIVar = DII->getVariable();
diff --git a/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp b/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
index 531b0a624dafab6..99c033cab64f803 100644
--- a/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
+++ b/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
@@ -321,8 +321,9 @@ void MemoryOpRemark::visitVariable(const Value *V,
   bool FoundDI = false;
   // Try to get an llvm.dbg.declare, which has a DILocalVariable giving us the
   // real debug info name and size of the variable.
-  for (const DbgVariableIntrinsic *DVI :
-       FindDbgDeclareUses(const_cast<Value *>(V))) {
+  SmallVector<DbgDeclareInst *> DbgDeclares;
+  findDbgDeclares(DbgDeclares, const_cast<Value *>(V));
+  for (const DbgVariableIntrinsic *DVI : DbgDeclares) {
     if (DILocalVariable *DILV = DVI->getVariable()) {
       std::optional<uint64_t> DISize = getSizeInBytes(DILV->getSizeInBits());
       VariableInfo Var{DILV->getName(), DISize};

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented on 2nd commit; I think we need a change to avoid a bit of technically undefined behaviour here. It's unlikely that anything's going to optimise around it, but we should avoid relying on it in the compiler itself.

Otherwise this all looks good.

if (!MDV)
return;
template <typename IntrinsicT, bool AnyType,
DPValue::LocationType Type = DPValue::LocationType(-1)>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://eel.is/c++draft/expr.static.cast para 10 indicates that this may be undefined behaviour. IMO we should bite the bullet and add an "End" type to the enumeration, followed by an "Any" type that's explicitly to be used in APIs like this. Slightly less than ideal, but at least well formed.
I'd also suggest "AnyKind" rather than "AnyType", to avoid mixing the ideas of C++ types and DPValue stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-edit: that is, casting -1 to DPValue::LocationType, an enumeration that doesn't have an enumerated element for -1, is likely undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. Given it's already qualified by an enum class name LocationType I think Any should suffice? I'm not sure we need the End marker, but I've put it in there anyway.

We could use bit flags (e.g. Any = 1 | 2)- I've no strong opinion either way and have not done so in the latest revsiion.

wdyt?

OCHyams added a commit that referenced this pull request Dec 12, 2023
…es (#73498)

This simplifies an upcoming patch to support the RemoveDIs project (tracking
variable locations without using intrinsics).

Next in this series is #73500.
This change doesn't change any call sites.
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- I'm aware of a convention where you have the "End" enumeration name take the same value as the final enumeration name, so End = Value,, so that you don't need to have a useless "case End:" in any switch statements. However, that might be a C thing rather than a C++ thing, so feel free to ignore.

@OCHyams
Copy link
Contributor Author

OCHyams commented Dec 12, 2023

I've left it with a default value as Any suffers the same problem

@OCHyams OCHyams merged commit 17b8f87 into llvm:main Dec 12, 2023
3 of 4 checks passed
OCHyams added a commit that referenced this pull request Dec 12, 2023
This patch doesn't change any call sites.

Depends on #73498.

Reverted in 87c6867.
OCHyams added a commit that referenced this pull request Dec 13, 2023
As part of the RemoveDIs project, transitioning to non-instruction debug
info, all debug intrinsic handling code needs to be duplicated to handle
DPValues.

--try-experimental-debuginfo-iterators enables the new debug mode in
tests if the CMake option has been enabled.

`getInsertPtAfterFramePtr` now returns an iterator so we don't lose
debug-info-communicating bits.

---

Depends on #73500, #74090, #74091.
OCHyams added a commit to OCHyams/llvm-project that referenced this pull request Dec 13, 2023
@OCHyams OCHyams deleted the ddd/finddbgdeclares2 branch January 5, 2024 10:25
ichaer added a commit to ichaer/llvm-project-onesided_lower_bound that referenced this pull request Jan 29, 2024
* main: (5908 commits)
  [readtapi] Cleanup printing command line options (llvm#75106)
  [flang] Fix compilation error due to variable no being used (llvm#75210)
  [C API] Add getters and setters for fast-math flags on relevant instructions (llvm#75123)
  [libc++][CI] Tests the no RTTI configuration. (llvm#65518)
  [RemoveDIs] Fold variable into assert, it's only used once. NFC
  [RemoveDI] Handle DPValues in SROA (llvm#74089)
  [AArch64][GlobalISel] Test Pre-Commit for Look into array's element
  [mlir][tensor] Fix bug in `tensor.extract(tensor.from_elements)` folder (llvm#75109)
  [analyzer] Move alpha checker EnumCastOutOfRange to optin (llvm#67157)
  [RemoveDIs] Handle DPValues in replaceDbgDeclare (llvm#73507)
  [SHT_LLVM_BB_ADDR_MAP] Implements PGOAnalysisMap in Object and ObjectYAML with tests.
  [X86][GlobalISel] Add instruction selection for G_SELECT (llvm#70753)
  [AMDGPU] Remove unused function splitScalar64BitAddSub
  [LLVM][DWARF] Add compilation directory and dwo name to TU in dwo section (llvm#74909)
  [clang][Interp] Implement __builtin_ffs (llvm#72988)
  [RemoveDIs] Update ConvertDebugDeclareToDebugValue after llvm#72276 (llvm#73508)
  [libc][NFC] Reuse `FloatProperties` constant instead of creating new ones (llvm#75187)
  [RemoveDIs] Fix removeRedundantDdbgInstrs utils for dbg.declares (llvm#74102)
  Reapply "[RemoveDIs][NFC] Find DPValues using findDbgDeclares  (llvm#73500)"
  [GitHub] Remove author association print from new-prs workflow
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants