[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 {
-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
More information about the llvm-commits
mailing list