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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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;
----------------
xur wrote:
> 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.
While it wouldn't break any build system I know about - it would break, for instance, in C++ standard modules (where declarations are owned by the module that declares them) and violates at least conceptual layering, that Transforms have nothing to do with MIR.

(Google's style guide talks about not forward declaring entities not in the same project - though what constitutes a "project" is a bit vague and probably doesn't quite apply here)


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:63
+
+template <class BlockT> struct TypeMap {};
+template <> struct TypeMap<BasicBlock> {
----------------
xur wrote:
> 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.
> TypeMap specialization needs to after the TypeMap definition, but before the uses of the types.

I don't think that's the case. At least a small test case ( https://godbolt.org/z/ecjox8 ) appears to work as desired - and I'd expect /something/ like that to work, since it'd be quite common to specialize a template far away from where that template is defined and used, since it might need to be specialized with some type unknown to the template and the usage (as is/should be the case here).


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

https://reviews.llvm.org/D96981