[PATCH] D43285: [CodeGen] Refactor AppleAccelTable

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 16 08:42:30 PST 2018

JDevlieghere added a comment.

LGTM. Thanks for working on this Pavel, I'm very happy with the direction this is going.

Comment at: include/llvm/CodeGen/AccelTable.h:117
-/// Apple-style accelerator table base class.
-class AppleAccelTableBase {
+namespace detail {
+/// A base class holding non-template-dependant functionality of the AccelTable
labath wrote:
> JDevlieghere wrote:
> > What was your motivation for wrapping this into a separate namespace? When I saw `detail` I assumed this is where the differences between the Apple and DWARF tables would live. Maybe I'm missing something  but otherwise I think we'd be better of without it.  
> It's customary (at least in some parts of the llvm codebase) to place the things which have to be declared in the header, but that are not really considered to be a public interface into the `detail` namespace. I've put it here because I don't think anyone on the outside should ever refer to this non-templated class directly as doing so (particularly in combination with the non-templated version of emitAppleAccelTable) would allow you to circumvent some of the type safety checks (and probably crash).
> But it seems that's not how things are done here (this is the only instance of the detail namespace in CodeGen), so I've removed it.
Alright, thanks for explaining. I didn't know about the `detail` namespace. 

Comment at: include/llvm/CodeGen/AccelTable.h:132
-    HashData(DwarfStringPoolEntryRef Name) : Name(Name) {
-      HashValue = djbHash(Name.getString());
-    }
+    HashData(DwarfStringPoolEntryRef Name, HashFn *Hash)
+        : Name(Name), HashValue(Hash(Name.getString())) {}
labath wrote:
> JDevlieghere wrote:
> > Is there any advantage over passing in the hash value directly?
> We delay computing the hash in the `try_emplace` function (in `AddName`) until we know that we really need to construct a new HashData object (i.e., that this is a new name).
Makes sense!

Comment at: include/llvm/CodeGen/AccelTable.h:150
-  using HashList = std::vector<HashData *>;
-  HashList Hashes;
+  HashFn *Hash;
+  uint32_t BucketCount;
labath wrote:
> JDevlieghere wrote:
> > `std::function<uint32_t(StringRef)>`?
> I think we should avoid that as `std::function` is fairly heavy (it uses virtual dispatch and what-not). Besides, in the current model, we obtain the hash function as `&DataT::hash`, so this cannot really be anything other than a simple function pointer.
Fair enough. 



More information about the llvm-commits mailing list