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

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added inline comments.


================
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:
> > > 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.
ah. OK. you are right!
I got the point now.

I will update the patch shortly.


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

https://reviews.llvm.org/D96981