[PATCH] D42334: [NFC] Refactor Apple Accelerator Tables

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:178
     DwarfStringPoolEntryRef Name;
-    std::vector<HashDataContents *> Values;
+    std::vector<DataT *> Values;
   };
----------------
aprantl wrote:
> JDevlieghere wrote:
> > aprantl wrote:
> > > Reading just the struct definition I'm a little puzzled why this needs to be a template if we are only storing a pointer to DataT. i wonder if we could get away with just templateizing one or two methods instead of the entire class.
> > I don't think there's anything preventing this, I just liked this approach better because:
> > 
> >  1. No overhead of virtual calls.
> >  2. No risk of accidentally mixing different types of data in one table. (make interfaces easy to use correctly and hard to use incorrectly)
> >  3. No need to explicitly specify the atoms in the constructor and having it abstracted and self contained in the templated class. (same reason as 2)
> > 
> > Because of (2) and (3) it makes sense to have an `emit` interface where you call the appropriate methods to emit the data, because the atoms are right there and you have a guarantee about their form. If this is not the case, I think we should go for an interface similar to D42246 where DataT has an interface to get the data for a given atom and emit the correct form based on the atom's form; which means a loop and more function calls.
> > 
> > tl;dr it's an interface design decision and not a technical limitation.
> I'm mostly worried about code size. My understanding (please let me know if this is misinformed) is that by having the entire class be a template we will get N copies of all methods including the ones that don't depend on the templated type such as the constructor.
> > No overhead of virtual calls.
> Do you mean virtual calls when calling methods on the Values stored in the vector?
> You could templateize the emit method itself and/or create a non-template AppleAccelTableBase class that contains all the generic code and templated AppleAccelTable<DataT> inheriting from that class. That's a common pattern I've seen used all over LLVM.
Indeed, the virtual call would be the `::emit()` method in `DataT`, so one indirect call for every entry in the list. Inheriting from a common base class isn't really feasible either, as every function (indirectly) depends on `DataT`.

As we discussed offline, I share your code size concerns. There should be at most 3 different instantiations of the template, so the growth is limited as long as no new configuration of tables are added, which is a realistic assumption given the existence of DWARFv5 accelerator tables. This also means that as we move to the DWARFv5 implementation on darwin, this will becomes less of a hot path and it might be worth settling for a little runtime overhead to save on code size. 

I encourage everyone to share which of the two trade-offs they feel we should pursue.


Repository:
  rL LLVM

https://reviews.llvm.org/D42334