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.

Sign up for GitHub

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

[NFC] Change FindDbgDeclareUsers interface to match findDbgUsers/values #73498

Merged
merged 3 commits into from
Dec 12, 2023
Merged

[NFC] Change FindDbgDeclareUsers interface to match findDbgUsers/values #73498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4964746
[NFC] Change FindDbgDeclareUsers interface to match findDbgUsers/values
OCHyams Nov 17, 2023
610796c
set SmallVector size to 1
OCHyams Dec 11, 2023
f4c0606
rm shadowing variable
OCHyams Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/include/llvm/IR/DebugInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ 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);
Copy link
Contributor

@felipepiovezan felipepiovezan Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Forgive me if I mentioned this in another patch (I couldn't find it), but why insist on using out parameters instead of returning a pair of vectors?
Out parameters made a lot of sense in old C++, but nowadays not so much. Out parameters just encourage variables with larger lifetimes and those interfaces create questions like "can this handle a pre-populated container? Will it erase the container?". It also makes callers need to know to about the input type of the container (note how previously the code did not need to know about the type of the container being returned).

I realize you will later have one of the out parameters be optional, but this could be accomplished with multiple functions. For example, we could minimize a lot of the diffs in this stack of patches by having differently named functions that share an implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out parameters also encourage non-constness of variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that avoiding out-parameters would be ideal.

I chose out-params here for consistency. findDbgValues has always had an out parameter and recent DPValue patches added a second one for DPValues. So it seemed better to follow suit with findDbgDeclares rather than have two different interfaces for similar functions.

this could be accomplished with multiple functions

IMO that would make it easier for the old and new debug modes to diverge - having a single "find all debug-things" function seemed to make sense from that point of view.

One final point - we're in a bit of a transitionary stage. We would like to be able to completely remove debug intrinsics soon, at which point these functions go back to having a single "return value". Perhaps worth refactoring at that point?

I don't have a really strong opinion on this - I just wanted to show my working so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI

Forgive me if I mentioned this in another patch (I couldn't find it), but why insist on using out parameters instead of returning a pair of vectors?

It was in #74480, for a similar function.

Copy link
Contributor

@felipepiovezan felipepiovezan Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is a transitionary state, this is probably fine. But you know how LLVM's transitionary states can usually last a few years...

I chose out-params here for consistency. findDbgValues has always had an out parameter

Consistency is a good argument for sure. It is a bit unfortunate because we had the chance to modify the bad API and make it identical to the good API, instead of changing the good API to match the bad API. Now we have two extra steps before we can have good APIs all around

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI

Forgive me if I mentioned this in another patch (I couldn't find it), but why insist on using out parameters instead of returning a pair of vectors?

It was in #74480, for a similar function.

Ohh thank you! That reminds me I need to get back to that review. I keep clicking the "pull requests" view of Github hoping it will show all reviews I am involved in, but sadly it doesn't :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may well already be aware, but just in case it helps - If you go to to to Notifications (top right, second icon from the right), you can click the "Review Requested" option. I don't know if this shows you pull requests where you've commented but are not a reviewer though... I guess they probably just float around in the notification inbox.


/// Finds the llvm.dbg.value intrinsics describing a value.
void findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues,
Expand Down
14 changes: 6 additions & 8 deletions llvm/lib/IR/DebugInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,23 @@ using namespace llvm;
using namespace llvm::at;
using namespace llvm::dwarf;

TinyPtrVector<DbgDeclareInst *> llvm::FindDbgDeclareUses(Value *V) {
void llvm::findDbgDeclares(SmallVectorImpl<DbgDeclareInst *> &DbgUsers,
Value *V) {
// This function is hot. Check whether the value has any metadata to avoid a
// DenseMap lookup.
if (!V->isUsedByMetadata())
return {};
return;
auto *L = LocalAsMetadata::getIfExists(V);
if (!L)
return {};
return;
auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L);
if (!MDV)
return {};
return;

TinyPtrVector<DbgDeclareInst *> Declares;
for (User *U : MDV->users()) {
if (auto *DDI = dyn_cast<DbgDeclareInst>(U))
Declares.push_back(DDI);
DbgUsers.push_back(DDI);
}

return Declares;
}

template <typename IntrinsicT>
Expand Down
12 changes: 8 additions & 4 deletions llvm/lib/Transforms/Coroutines/CoroFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,8 @@ static void cacheDIVar(FrameDataInfo &FrameData,
if (DIVarCache.contains(V))
continue;

auto DDIs = FindDbgDeclareUses(V);
SmallVector<DbgDeclareInst *, 1> DDIs;
findDbgDeclares(DDIs, V);
auto *I = llvm::find_if(DDIs, [](DbgDeclareInst *DDI) {
return DDI->getExpression()->getNumElements() == 0;
});
Expand Down Expand Up @@ -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 *, 1> DIs;
findDbgDeclares(DIs, PromiseAlloca);
if (DIs.empty())
return;

Expand Down Expand Up @@ -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 *, 1> 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.
Expand All @@ -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);
}
}

Expand Down
13 changes: 10 additions & 3 deletions llvm/lib/Transforms/Scalar/SROA.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *, 1> 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);
Expand Down Expand Up @@ -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 *, 1> FragDbgDeclares;
findDbgDeclares(FragDbgDeclares, Fragment.Alloca);
for (DbgDeclareInst *OldDII : FragDbgDeclares) {
auto SameVariableFragment = [](const DbgVariableIntrinsic *LHS,
const DbgVariableIntrinsic *RHS) {
return LHS->getVariable() == RHS->getVariable() &&
Expand Down Expand Up @@ -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 *, 1> DbgDeclares;
findDbgDeclares(DbgDeclares, AI);
for (DbgDeclareInst *OldDII : DbgDeclares)
OldDII->eraseFromParent();
}

Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Transforms/Utils/Local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *, 1> DbgDeclares;
findDbgDeclares(DbgDeclares, Address);
for (DbgVariableIntrinsic *DII : DbgDeclares) {
const DebugLoc &Loc = DII->getDebugLoc();
auto *DIVar = DII->getVariable();
Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *, 1> 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};
Expand Down