IRSymtab: Use StringSet instead of DenseMap for preserved symbols

[arsenm]

Microbenchmarking shows this is faster

[arsenm]

• RuntimeLibcalls: Fix building hash table with duplicate entries #153801
• RuntimeLibcalls: Generate table of libcall name lengths #153210
• RuntimeLibcalls: Return StringRef for libcall names #153209
• TableGen: Emit static hash table for runtime libcalls #150192
• IRSymtab: Use StringSet instead of DenseMap for preserved symbols #149836 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

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

Author: Matt Arsenault (arsenm)

Changes

Microbenchmarking shows this is faster

---

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

1 Files Affected:

• (modified) llvm/lib/Object/IRSymtab.cpp (+6-5)

diff --git a/llvm/lib/Object/IRSymtab.cpp b/llvm/lib/Object/IRSymtab.cpp
index 2579fa37935f0..0f194953787e6 100644
--- a/llvm/lib/Object/IRSymtab.cpp
+++ b/llvm/lib/Object/IRSymtab.cpp
@@ -8,11 +8,11 @@
 
 #include "llvm/Object/IRSymtab.h"
 #include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Comdat.h"
@@ -213,9 +213,10 @@ Expected<int> Builder::getComdatIndex(const Comdat *C, const Module *M) {
   return P.first->second;
 }
 
-static DenseSet<StringRef> buildPreservedSymbolsSet(const Triple &TT) {
-  DenseSet<StringRef> PreservedSymbolSet(std::begin(PreservedSymbols),
-                                         std::end(PreservedSymbols));
+static StringSet<> buildPreservedSymbolsSet(const Triple &TT) {
+  StringSet<> PreservedSymbolSet;
+  PreservedSymbolSet.insert(std::begin(PreservedSymbols),
+                            std::end(PreservedSymbols));
   // FIXME: Do we need to pass in ABI fields from TargetOptions?
   RTLIB::RuntimeLibcallsInfo Libcalls(TT);
   for (RTLIB::LibcallImpl Impl : Libcalls.getLibcallImpls()) {
@@ -280,7 +281,7 @@ Error Builder::addSymbol(const ModuleSymbolTable &Msymtab,
 
   setStr(Sym.IRName, GV->getName());
 
-  static const DenseSet<StringRef> PreservedSymbolsSet =
+  static const StringSet<> PreservedSymbolsSet =
       buildPreservedSymbolsSet(GV->getParent()->getTargetTriple());
   bool IsPreservedSymbol = PreservedSymbolsSet.contains(GV->getName());
 

[nikic]

Not seeing a difference on llvm-compile-time-tracker: https://llvm-compile-time-tracker.com/compare.php?from=c9fe19a99bf41c165524dcb3e9ff939527b5178b&to=ffc6ababf334db5126144f263e3b0cada44ed751&stat=instructions:u

I'm somewhat surprised that StringSet is faster than DenseSet<StringRef>, as the former will copy all the strings. I wonder whether this comes down to having a cached hash, in which case DenseSet<CachedHashStringRef> may be best.

Anyway, no objections if you have data that this is indeed better.