-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
[LLVM][DWARF] Make dwarf::getDebugNamesBucketCount return a pair. #83047
Conversation
llvm::dwarf::getDebugNamesBucketCount directly returns the bucket count, via return statement, but it also returns the hash count via a parameter. This changes the function to return them both as a std::pair, in the return statement. It also changes the name of the function to make it clear it returns both values.
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-binary-utilities Author: None (cmtice) Changesllvm::dwarf::getDebugNamesBucketCount directly returns the bucket count, via return statement, but it also returns the hash count via a parameter. This changes the function to return them both as a std::pair, in the return statement. It also changes the name of the function to make it clear it returns both values. Full diff: https://github.com/llvm/llvm-project/pull/83047.diff 2 Files Affected:
diff --git a/llvm/include/llvm/BinaryFormat/Dwarf.h b/llvm/include/llvm/BinaryFormat/Dwarf.h
index 44c0030251b37e..e2d79785162e37 100644
--- a/llvm/include/llvm/BinaryFormat/Dwarf.h
+++ b/llvm/include/llvm/BinaryFormat/Dwarf.h
@@ -616,20 +616,21 @@ enum AcceleratorTable {
// Uniquify the string hashes and calculate the bucket count for the
// DWARF v5 Accelerator Table. NOTE: This function effectively consumes the
// 'hashes' input parameter.
-inline uint32_t getDebugNamesBucketCount(MutableArrayRef<uint32_t> hashes,
- uint32_t &uniqueHashCount) {
- uint32_t BucketCount = 0;
+inline std::pair<uint32_t, uint32_t> getDebugNamesBucketAndHashCount(
+ MutableArrayRef<uint32_t> hashes) {
+ uint32_t uniqueHashCount = 0;
+ uint32_t bucketCount = 0;
sort(hashes);
uniqueHashCount = llvm::unique(hashes) - hashes.begin();
if (uniqueHashCount > 1024)
- BucketCount = uniqueHashCount / 4;
+ bucketCount = uniqueHashCount / 4;
else if (uniqueHashCount > 16)
- BucketCount = uniqueHashCount / 2;
+ bucketCount = uniqueHashCount / 2;
else
- BucketCount = std::max<uint32_t>(uniqueHashCount, 1);
+ bucketCount = std::max<uint32_t>(uniqueHashCount, 1);
- return BucketCount;
+ return {bucketCount, uniqueHashCount};
}
// Constants for the GNU pubnames/pubtypes extensions supporting gdb index.
diff --git a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
index 23fc9b2e0410e0..94e9d5e9cb77ce 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
@@ -38,7 +38,9 @@ void AccelTableBase::computeBucketCount() {
for (const auto &E : Entries)
Uniques.push_back(E.second.HashValue);
- BucketCount = llvm::dwarf::getDebugNamesBucketCount(Uniques, UniqueHashCount);
+ auto counts = llvm::dwarf::getDebugNamesBucketAndHashCount(Uniques);
+ BucketCount = counts.first;
+ UniqueHashCount = counts.second;
}
void AccelTableBase::finalize(AsmPrinter *Asm, StringRef Prefix) {
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
uint32_t BucketCount = 0; | ||
inline std::pair<uint32_t, uint32_t> getDebugNamesBucketAndHashCount( | ||
MutableArrayRef<uint32_t> hashes) { | ||
uint32_t uniqueHashCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid declaring-then-initializing as much as possible, this should be moved to where the variable is actually computed:
uint32_t uniqueHashCount = llvm::unique(hashes) - hashes.begin();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. :-)
Fix clang-format issue. Avoid unnecessary declaration before initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
uint32_t BucketCount = 0; | ||
inline std::pair<uint32_t, uint32_t> | ||
getDebugNamesBucketAndHashCount(MutableArrayRef<uint32_t> hashes) { | ||
uint32_t bucketCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the case? Doesn't LLVM use UpperCamelCase for variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I had missed this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought LLVM variables were supposed to start with lower case? So I thought I was fixing a problem? I can switch it back if you think I should...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably thinking of functions, which (unlink LLDB) use lowerCamelCase
: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done.
I suggested |
Make local variables start with uppercase letter.
Make local variables start with uppercase letter.
@@ -38,7 +38,9 @@ void AccelTableBase::computeBucketCount() { | |||
for (const auto &E : Entries) | |||
Uniques.push_back(E.second.HashValue); | |||
|
|||
BucketCount = llvm::dwarf::getDebugNamesBucketCount(Uniques, UniqueHashCount); | |||
auto Counts = llvm::dwarf::getDebugNamesBucketAndHashCount(Uniques); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to std::tie(BucketCount, UniqueHashCount)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this - @cmtice perhaps you could make this change as a follow-up commit. No need to send it for review. (but please follow up on this comment thread and mention the commit hash of the change once it's upstream)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the simplification already. Sometimes reviewers may click "Accept" and expect some minor code suggestions will be adopted before landing. It's worth paying attention to these comments.
(For both #82153 and this #83047, my comments were created before the patch landed. Don't worry. We all made mistakes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, 7b0b64a cool, thanks!
Fix case of parameter in comment.
The buildkite build failure appears to have no relation to this change. Does anybody know how I can force the build test to re-run, so that I can get a green "Squash and merge" button? |
You can still click the button, the failure seems unrelated indeed |
The code suggestion was neglected when the patch landed.
llvm::dwarf::getDebugNamesBucketCount directly returns the bucket count, via return statement, but it also returns the hash count via a parameter. This changes the function to return them both as a std::pair, in the return statement. It also changes the name of the function to make it clear it returns both values.