This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Refactor AppleAccelTable
ClosedPublic

Authored by labath on Feb 14 2018, 5:17 AM.

Details

Summary

This commit separates the abstract accelerator table data structure
from the code for writing out an on-disk representation of a specific
accelerator table format. The idea is that former (now called
AccelTable<T>) can be reused for the DWARF v5 accelerator tables
as-is, without any further customizations.

Some bits of the emission code (now living in the EmissionContext class)
can be reused for DWARF v5 as well, but the subtle differences in the
layout of various subtables mean the sharing is not always possible.
(Also, the individual emit*** functions are fairly simple so there's a
tradeoff between making a bigger general-purpose function, and two
smaller targeted functions.)

Another advantage of this setup is that more of the serialization logic
can be hidden in the .cpp file -- I have moved declarations of the
header and all the emission functions there.

Diff Detail

Event Timeline

labath created this revision.Feb 14 2018, 5:17 AM

I've also put up D43286 (which is still WIP, but nearing completion...) so you can see the context of this change.

labath updated this revision to Diff 134212.Feb 14 2018, 5:47 AM

delete copy constructors of AccelTableBase and move one comment a bit.

Looks good, with a few minor comments inline.

I think it would be great if we can add a small diagram or textual explanation to the top of the header file with the way the classes are structure, similar to your summary here. The comments on the individual classes are clear enough imho, but I can imagine that the big picture is a little harder to get when you don't know why we have all these different classes.

include/llvm/CodeGen/AccelTable.h
133–140

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.

147–148

Is there any advantage over passing in the hash value directly?

165–167

std::function<uint32_t(StringRef)>?

lib/CodeGen/AsmPrinter/AccelTable.cpp
90

[opinion] I think AccelTableEmitter or something like that would better convey the purpose of this class.

labath added inline comments.Feb 16 2018, 6:15 AM
include/llvm/CodeGen/AccelTable.h
133–140

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.

147–148

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).

165–167

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.

lib/CodeGen/AsmPrinter/AccelTable.cpp
90

I agree. (It's called this way because this started out just as a plain holder for the various bits needed for serialization).

labath updated this revision to Diff 134601.Feb 16 2018, 6:16 AM

Remove the detail namespace, and rename the EmissionContext class.

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

include/llvm/CodeGen/AccelTable.h
133–140

Alright, thanks for explaining. I didn't know about the detail namespace.

147–148

Makes sense!

165–167

Fair enough.

JDevlieghere accepted this revision.Feb 16 2018, 8:42 AM
This revision is now accepted and ready to land.Feb 16 2018, 8:42 AM
This revision was automatically updated to reflect the committed changes.