[PATCH] D111750: [llvm-profgen] Allow unsymbolized profile as perf input

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:659
+*/
+class UnsymbolizedProfileReader : public PerfReaderBase {
+public:
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > hoy wrote:
> > > > > What is the main reason of making this type hierarchy? It looks like `UnsymbolizedProfileReader` doesn't need most of the interfaces `PerfReaderBase` provides. Conceptually it sounds to me that the two classes function independently, and if we'd like code sharing,  `UnsymbolizedProfileReader` can be made the base class of `PerfReaderBase` or make a new base class that simply reads in something and outputs an symbolized profile?
> > > > > UnsymbolizedProfileReader can be made the base class of PerfReaderBase
> > > > 
> > > > I don't think this is a good idea. Conceptually, PerfReader is not a special kind of UnsymbolizedProfileReader.
> > > Right, they are independent of each other. PerfReader really deals with perf input. They only share in the raw output writting. A new base class makes more sense?
> > It seems to me the hierarchy should be  like 
> > ```
> > PerfReaderBase.  --->   PerfScriptReader.   ---> LBRPerfReader
> >                                             ---> HybridPerfReader
> > 
> >                  --->   UnsymbolizedProfileReader
> > ```
> > This will make the hierarchy deep though.
> > 
> That's the current hierarchy, IIUC? I was wondering if `UnsymbolizedProfileReader` should inherit from a base class that's not `PerfReaderBase`. If you look at the implementation of `PerfReaderBase`, there isn't much `UnsymbolizedProfileReader` shares. The writing of unsymbolized profile is probably not needed for `UnsymbolizedProfileReader`.
Yeah, It's mostly similar to your suggestion. I meant just rename current `PerfReaderBase` to `PerfScriptReader`. then we can use `PerfReaderBase` as the new base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111750