[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:63
+
+template <class BlockT> struct TypeMap {};
+template <> struct TypeMap<BasicBlock> {
----------------
xur wrote:
> 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. 
> 
> 
Sorry I'm not quite following.

Why would the MIR header file need to go before the template header file? In my example, all the MIR related things (struct b, and traits<b> specialization) came after the generic template/IR things.

I think the declaration of TypeMap/IRTraits could remain in SampleProfileLoaderBaseImpl.h for now.


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

https://reviews.llvm.org/D96981