Skip to content

[LLVM][DWARF] Refactor code for generating DWARF V5 .debug_names acce… #82264

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

Closed
wants to merge 3 commits into from
Closed

[LLVM][DWARF] Refactor code for generating DWARF V5 .debug_names acce… #82264

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1d522c
[LLVM][DWARF] Refactor code for generating DWARF V5 .debug_names acce…
cmtice Feb 18, 2024
cd40e59
[LLVM][DWARF] Refactir code fir generating DWARF v5 .debug_names acce…
cmtice Feb 20, 2024
9a9351d
Merge branch 'main' into llvm-unique
cmtice Feb 20, 2024
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
5 changes: 1 addition & 4 deletions llvm/include/llvm/ADT/GenericUniformityImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@

#include "llvm/ADT/GenericUniformityInfo.h"

#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SparseBitVector.h"
#include "llvm/ADT/StringExtras.h"
Expand All @@ -57,10 +58,6 @@

namespace llvm {

template <typename Range> auto unique(Range &&R) {
return std::unique(adl_begin(R), adl_end(R));
}

/// Construct a specially modified post-order traversal of cycles.
///
/// The ModifiedPO is contructed using a virtually modified CFG as follows:
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/ADT/STLExtras.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1994,6 +1994,10 @@ auto unique(Range &&R, Predicate P) {
return std::unique(adl_begin(R), adl_end(R), P);
}

template <typename Range> auto unique(Range &&R) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make this change (moving unique(Range&&) to STLExtras, and adding unit test coverage) in a separate commit (per https://llvm.org/docs/DeveloperPolicy.html#incremental-development )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: #82312

return std::unique(adl_begin(R), adl_end(R));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is just moved code, but all functions in STLExtras have to be documented, even if they just defer to the stl documentation; see an example on the function just below this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, in the new PR (requested by dwblaikie): #82312


/// Wrapper function around std::equal to detect if pair-wise elements between
/// two ranges are the same.
template <typename L, typename R> bool equal(L &&LRange, R &&RRange) {
Expand Down
18 changes: 18 additions & 0 deletions llvm/include/llvm/BinaryFormat/Dwarf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,24 @@ enum AcceleratorTable {
DW_hash_function_djb = 0u
};

// Uniquify the string hashes and calculate the bucket count for the
// DWARF v5 Accelerator Table.
inline uint32_t
ComputeDebugNamesUniqueHashes(SmallVector<uint32_t, 0> &hashes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We avoid putting the SmallSize of SmallVectors in the interface, using SmallVectorImpl instead.
However, this function should just use a MutableArrayRef<uint32_t> instead, to communicate the fact that we are not inserting nor removing from the vector.

Copy link
Member

Choose a reason for hiding this comment

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

New functions should use the functionName convention. (Older code may use FunctionName, which should be ignored.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see SmallVector<uint32_t, 0>? forgot to push maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, you're right! I've having a bit of trouble with my git push right now :-( -- git is rejecting the push: "Updates were rejected because the remote contains work that you do not have locally..." I tried a 'git merge' but that didn't fix the issue. I hope I don't have to abandon this branch & PR and create a new one. If you know how to fix this, please let me know! Otherwise, I'll keep you posted.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you show me the output of git status and the first few lines of git log --oneline --graph I might be able to tell you what's happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ git status
On branch llvm-unique
Untracked files:
...
nothing added to commit but untracked files present (use "git add" to track)
$ git log --oneline --graph* 260397dcb8e4 (HEAD -> llvm-unique) [LLVM][DWARF] Refactor code for generating DWARF V4 .debug_names accelerator table

...

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've given up and created a new pull request - it's simpler than fighting with git.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try to merge with the other branch where you did the ADT work? My guess is that the two branches had different bases, and if you merged without rebasing, your commits ended up buried in other commits from main.

uint32_t BucketCount = 0;

llvm::sort(hashes);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT we are already inside namespace llvm. (same comment below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

uint32_t uniqueHashCount = llvm::unique(hashes) - hashes.begin();
if (uniqueHashCount > 1024)
BucketCount = uniqueHashCount / 4;
else if (uniqueHashCount > 16)
BucketCount = uniqueHashCount / 2;
else
BucketCount = std::max<uint32_t>(uniqueHashCount, 1);

return BucketCount;
}

// Constants for the GNU pubnames/pubtypes extensions supporting gdb index.
enum GDBIndexEntryKind {
GIEK_NONE,
Expand Down
14 changes: 2 additions & 12 deletions llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,12 @@ using namespace llvm;

void AccelTableBase::computeBucketCount() {
// First get the number of unique hashes.
std::vector<uint32_t> Uniques;
SmallVector<uint32_t, 0> Uniques;
Uniques.reserve(Entries.size());
for (const auto &E : Entries)
Uniques.push_back(E.second.HashValue);
array_pod_sort(Uniques.begin(), Uniques.end());
std::vector<uint32_t>::iterator P =
std::unique(Uniques.begin(), Uniques.end());

UniqueHashCount = std::distance(Uniques.begin(), P);

if (UniqueHashCount > 1024)
BucketCount = UniqueHashCount / 4;
else if (UniqueHashCount > 16)
BucketCount = UniqueHashCount / 2;
else
BucketCount = std::max<uint32_t>(UniqueHashCount, 1);
BucketCount = llvm::dwarf::ComputeDebugNamesUniqueHashes(Uniques);
}

void AccelTableBase::finalize(AsmPrinter *Asm, StringRef Prefix) {
Expand Down
13 changes: 13 additions & 0 deletions llvm/unittests/ADT/STLExtrasTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,19 @@ TEST(STLExtras, Unique) {
EXPECT_EQ(3, V[3]);
}

TEST(STLExtras, UniqueNoPred) {
std::vector<uint32_t> V = {1, 5, 5, 4, 3, 3, 3};

auto I = llvm::unique(V);

EXPECT_EQ(I, V.begin() + 4);

EXPECT_EQ(1, V[0]);
EXPECT_EQ(5, V[1]);
EXPECT_EQ(4, V[2]);
EXPECT_EQ(3, V[3]);
}

TEST(STLExtrasTest, MakeVisitorOneCallable) {
auto IdentityLambda = [](auto X) { return X; };
auto IdentityVisitor = makeVisitor(IdentityLambda);
Expand Down