[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:
> 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)
Thanks for the explanation. Good to know this!


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:63
+
+template <class BlockT> struct TypeMap {};
+template <> struct TypeMap<BasicBlock> {
----------------
dblaikie wrote:
> 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).
Sorry that I did not make my point clear. I know the small test would work -- as it does have the problem I was talking about. 

The "TypeMap definition" is like line 1 in the small test case. It needs to be before the specialization at line 4 and line 12.
In my code, line 4 and line 12 will be in a different header file (or I can keep one in the template header, but the other will go the MIR header).
This MIR header file need to be before the template header file. But line 1 needs to be before the MIR header file.
So line 1 needs to be moved from the template header -- so that it appears before line 4 and line 12.

But this is not a problem. Because the layer violation issue, I need to change it anyway. 




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

https://reviews.llvm.org/D96981