[PATCH] D96981: [SampleFDO][NFC] Refactor: make SampleProfileLoaderBaseImpl a template class

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Really really broad feedback - this work to write common abstractions over LLVM IR and MIR seems to overlap with https://lists.llvm.org/pipermail/llvm-dev/2020-December/147433.html

It's non-trivial as to what exactly these APIs should look like (virtual/type erased v templates, etc). But may be useful to consider some of the options here.

My current rough guess is that templates are probably the right tool, but that more common abstractions should be implemented in the IR types themselves - eg: the TypeMap and getFunction/getEntryBB could be rolled into the IR types themselves as authoritative/core features, rather than grafting them on with traits.



================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:50-60
+class MachineInstr;
+class MachineBasicBlock;
+class MachineFunction;
+class MachineBlockFrequencyInfo;
+class MachineLoop;
+class MachineLoopInfo;
+class MachineOptimizationRemarkEmitter;
----------------
Seems like a layering violation to be declaring these entities that aren't reachable from Transform/Utils?

Could the MIR TypeMap specialization be defined in the MIR library instead?


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:63
+
+template <class BlockT> struct TypeMap {};
+template <> struct TypeMap<BasicBlock> {
----------------
This should probably be a declaration (it should mean more immediate/explicit compilation failures):
```
template<typename BlockT> struct TypeMap;
```
(also I tend to suggest using `typename` rather than `class` - since they don't have any different meaning for C++ and typename more accurately describes what kind of entity can be used (though, yes, in this particular case the typenames do have to be classes))


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:221-234
+namespace llvm {
+
+template <>
+Function &SampleProfileLoaderBaseImpl<BasicBlock>::getFunction(Function &F) {
+  return F;
+}
+
----------------
Perhaps these functions could be part of the TypeMap? (Generalizing TypeMap to "IRTraits" or something like that)

Alternatively they could be implemented as overloads, but part of traits is probably the right call.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96981/new/

https://reviews.llvm.org/D96981