[PATCH] D43285: [CodeGen] Refactor AppleAccelTable

[Jonas Devlieghere via Phabricator via llvm-commits]

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 {
-protected:
+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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D43285