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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:178
     DwarfStringPoolEntryRef Name;
-    std::vector<HashDataContents *> Values;
+    std::vector<DataT *> Values;
   };
----------------
JDevlieghere wrote:
> 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.
That is good point. Since this destined to become legacy code path that we will need to carry around for an indefinite time (because of backwards deployment), optimizing for code size sounds like the right approach to me. Does the implementation on opensource.apple.com also use virtual function calls or would this be a regression?


Repository:
  rL LLVM

https://reviews.llvm.org/D42334