[PATCH] D111750: [llvm-profgen] Allow unsymbolized profile as perf input
Hongtao Yu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 25 20:01:08 PDT 2021
hoy added inline comments.
================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:659
+*/
+class UnsymbolizedProfileReader : public PerfReaderBase {
+public:
----------------
wlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > wlei wrote:
> > > > 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.
> > > The hierarchy in the latest version looks good to me.
> > >
> > > However, I'm wondering if we could move LBRPerfReader::generateRawProfile into PerfScriptReader? That way we could have HybridPerfReader and LBRPerfReader both inherit from PerfScriptReader directly. The LBRPerfReader-> HybridPerfReader hierarchy seems a bit weird, but I understand it was for reusing LBRPerfReader::generateRawProfile. Don't bother if this can cause bigger change.
> > I now see what you mean. The renaming sounds good. It separates `UnsymbolizedProfileReader` which doesn't do anything with perf (perf data or perf script) from the old hierarchy. Perhaps rename the new `PerfReaderBase` to `ProfileReaderBase`?
> >
> > However, I'm wondering if we could move LBRPerfReader::generateRawProfile into PerfScriptReader?
>
> Sounds good, not a big change.
>
> > Perhaps rename the new PerfReaderBase to ProfileReaderBase?
>
> Just want to confirm this, do you mean also change the file name? currently the file is PerfReader.cpp/h. It seems the base class is used to read perf.data, perf script, unsymbolized profile, it has both `perf` and `profile` term, I don't have any preference though.
>
>
>
>
I see. I’m fine with the PerfReaderBase class name and PerfReader file names to minimize the changes. Perf is the main scenario we support.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111750/new/
https://reviews.llvm.org/D111750
More information about the llvm-commits
mailing list