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

cmtice · 2024-02-19T16:40:40Z

…lerator table.

Refactor the code that uniques the entries and computes the bucket count for the DWARF V5 .debug_names accelerator table. This will make it easier to share this bit of code wtih other tools such as LLD.

…lerator table. Refactor the code that uniques the entries and computes the bucket count for the DWARF V5 .debug_names accelerator table. This will make it easier to share this bit of code wtih other tools such as LLD.

llvmbot · 2024-02-19T16:41:14Z

@llvm/pr-subscribers-llvm-adt
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (cmtice)

Changes

…lerator table.

Refactor the code that uniques the entries and computes the bucket count for the DWARF V5 .debug_names accelerator table. This will make it easier to share this bit of code wtih other tools such as LLD.

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

5 Files Affected:

(modified) llvm/include/llvm/ADT/GenericUniformityImpl.h (+1-4)
(modified) llvm/include/llvm/ADT/STLExtras.h (+4)
(modified) llvm/include/llvm/BinaryFormat/Dwarf.h (+18)
(modified) llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp (+2-12)
(modified) llvm/unittests/ADT/STLExtrasTest.cpp (+13)

diff --git a/llvm/include/llvm/ADT/GenericUniformityImpl.h b/llvm/include/llvm/ADT/GenericUniformityImpl.h
index d397b937d78ccc..6b744384051b56 100644
--- a/llvm/include/llvm/ADT/GenericUniformityImpl.h
+++ b/llvm/include/llvm/ADT/GenericUniformityImpl.h
@@ -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"
@@ -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:
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index a136eeb0ff1bd7..3d2f4bb40b6b79 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -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) {
+  return std::unique(adl_begin(R), adl_end(R));
+}
+
 /// 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) {
diff --git a/llvm/include/llvm/BinaryFormat/Dwarf.h b/llvm/include/llvm/BinaryFormat/Dwarf.h
index 869352b35e3235..3a245972a2aa93 100644
--- a/llvm/include/llvm/BinaryFormat/Dwarf.h
+++ b/llvm/include/llvm/BinaryFormat/Dwarf.h
@@ -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) {
+  uint32_t BucketCount = 0;
+
+  llvm::sort(hashes);
+  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,
diff --git a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
index 22d995a9cc3c56..7dce65d668a6de 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
@@ -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) {
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index 7db339e4ef31cd..cafef5b5fad512 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -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);

felipepiovezan · 2024-02-19T17:19:16Z

llvm/include/llvm/ADT/STLExtras.h

@@ -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) {
+  return std::unique(adl_begin(R), adl_end(R));
+}


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

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

felipepiovezan · 2024-02-19T17:23:12Z

llvm/include/llvm/BinaryFormat/Dwarf.h

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


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.

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

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

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.

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.

$ 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

7f3980a (origin/main, origin/HEAD, main) [Fuzzer] Use user signal to coordinate handler shutdown ([Fuzzer] Use user signal to coordinate handler shutdown #82067)

d2942a8 [MergeFunctions] Fix thunks for non-instruction debug info ([MergeFunctions] Fix thunks for non-instruction debug info #82080)

4c6043d [clang][InstallAPI] Add input file support to library ([clang][InstallAPI] Add input file support to library #81701)

ae8facc [lldb-dap] Do not write over the existing error if launchCommands fail during debugger launch. ([lldb-dap] Do not write over the existing error if launchCommands fail during debugger launch. #82051)

4a23ab4 Fix Wcovered-switch-default Werror after [RemoveDIs][NFC] Introduce DbgRecord base class [1/3] #78252

ef28379 [llvm-objcopy] Fix file offsets when PT_INTERP/PT_LOAD offsets are equal ([llvm-objcopy] Fix file offsets when PT_INTERP/PT_LOAD offsets are equal #80562)

066773c [X86] computeKnownBitsForTargetNode - add generic handling of PSHUFB

94100bc [OpenMP][AIX]Add assembly file containing microtasking routines and unnamed common block definitions ([OpenMP][AIX]Add assembly file containing microtasking routines and unnamed common block definitions #81770)

3b7ba24 [C23] No longer assert on huge enumerator values ([C23] No longer assert on huge enumerator values #81760)

9923d29 [VPlan] Merge main VPlan verifer with HCFG verifier.

9facaad [ValueTracking] Improve tracking for constant range of {s|u}rem C, x

aa7076f [ValueTracking] Add tests for constant range of {s|u}rem C, x; NFC

...

I've given up and created a new pull request - it's simpler than fighting with git.

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.

felipepiovezan · 2024-02-19T17:24:36Z

llvm/include/llvm/BinaryFormat/Dwarf.h

+ComputeDebugNamesUniqueHashes(SmallVector<uint32_t, 0> &hashes) {
+  uint32_t BucketCount = 0;
+
+  llvm::sort(hashes);


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

Fixed, thanks.

dwblaikie · 2024-02-19T23:27:49Z

llvm/include/llvm/ADT/STLExtras.h

@@ -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) {


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 )

Done: #82312

…lerator table. Take the code for moving 'unique' from GenericUniformityImpl.h to STLExtras.h out of the change (to put it into a separate change).

cmtice · 2024-02-20T07:53:26Z

Note: This PR is now blocked by #82312; it won't compile properly until the other PR gets committed.

cmtice · 2024-02-20T18:17:29Z

I've gone ahead a created a new PR with all the latest updates addressing the review comments: #82394

cmtice · 2024-02-20T18:17:45Z

Closing this pull request in favor of the new one.

llvmbot added debuginfo llvm:binary-utilities llvm:adt labels Feb 19, 2024

felipepiovezan reviewed Feb 19, 2024

View reviewed changes

MaskRay requested a review from dwblaikie February 19, 2024 17:43

dwblaikie reviewed Feb 19, 2024

View reviewed changes

[LLVM][DWARF] Refactir code fir generating DWARF v5 .debug_names acce…

cd40e59

…lerator table. Take the code for moving 'unique' from GenericUniformityImpl.h to STLExtras.h out of the change (to put it into a separate change).

Merge branch 'main' into llvm-unique

9a9351d

cmtice mentioned this pull request Feb 20, 2024

[LLVM][DWARF] Refactor code for generating DWARF V5 .debug_names #82394

Merged

cmtice closed this Feb 20, 2024

cmtice deleted the llvm-unique branch February 22, 2024 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cmtice commented Feb 19, 2024

llvmbot commented Feb 19, 2024 •

edited

felipepiovezan Feb 19, 2024

cmtice Feb 20, 2024

felipepiovezan Feb 19, 2024

MaskRay Feb 19, 2024

cmtice Feb 20, 2024

felipepiovezan Feb 20, 2024

cmtice Feb 20, 2024

felipepiovezan Feb 20, 2024

cmtice Feb 20, 2024

cmtice Feb 20, 2024

pogo59 Feb 20, 2024

felipepiovezan Feb 19, 2024

cmtice Feb 20, 2024

dwblaikie Feb 19, 2024

cmtice Feb 20, 2024

cmtice commented Feb 20, 2024

cmtice commented Feb 20, 2024

cmtice commented Feb 20, 2024

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

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

Conversation

cmtice commented Feb 19, 2024

llvmbot commented Feb 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmtice commented Feb 20, 2024

cmtice commented Feb 20, 2024

cmtice commented Feb 20, 2024

llvmbot commented Feb 19, 2024 •

edited