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

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
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;
----------------
dblaikie wrote:
> 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?
why this is a layering violation? I think this is just a forward declaration -- we don't need the definition (complete type) here.


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:63
+
+template <class BlockT> struct TypeMap {};
+template <> struct TypeMap<BasicBlock> {
----------------
dblaikie wrote:
> 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))
TypeMap specialization needs to after the TypeMap definition, but before the uses of the types. 
That means I need to put the code in header, and split line 63 to a common header files. 
I think that is a littler messy than current way. 

using typename is a good idea. I will change.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:221-234
+namespace llvm {
+
+template <>
+Function &SampleProfileLoaderBaseImpl<BasicBlock>::getFunction(Function &F) {
+  return F;
+}
+
----------------
dblaikie wrote:
> 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.
I did not put into the TypeMap (or IRTraits) is because in current implementation, we don't have the complete type of the type parameters. 
If we put these functions into traits, we need to expose the definition.


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

https://reviews.llvm.org/D96981