[PATCH] D121655: [llvm-profgen] Read symbolized profiles for post-processing.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:730
 
+class SymbolizedProfileReader : public PerfReaderBase {
+public:
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > This doesn't look right. This is no longer a perf profile, but rather a proper LLVM profile, so inheriting from `PerfReaderBase` isn't good. As you can see, there's really no notion of trace, so `parsePerfTraces` really just calls LLVM's profile reader. 
> > > 
> > > Maybe instead of passing a reader to profile generator, we can just either pass the counters like we do today, or a ProfileMap which is what profile reader gives us. This may need two separate ctor for profile generator. But I think that's better than the (awkward) inheritance we have here.
> > I was thinking about that. Not only for the profile generator, the reader would also need a different constructor or a different type hierarchy and the work flow of llvm-profgen may not look as clean as now. As shown below (from `llvm-profgen::main`), the current approach tries to reuse the `PerfReaderBase` and `ProfileGeneratorBase`. I guess it's a tradeoff between adding a new base class for Reader or diverging the workflow structure a little bit.
> > 
> >  
> > ```
> >  PerfInputFile PerfFile = getPerfInputFile();
> >   std::unique_ptr<PerfReaderBase> Reader =
> >       PerfReaderBase::create(Binary.get(), PerfFile);
> >   // Parse perf events and samples
> >   Reader->parsePerfTraces();
> > 
> >   if (SkipSymbolization)
> >     return EXIT_SUCCESS;
> > 
> >   std::unique_ptr<ProfileGeneratorBase> Generator =
> >       ProfileGeneratorBase::create(Binary.get(), Reader->getSampleCounters(),
> >                                    Reader->profileIsCSFlat());
> >   Generator->generateProfile();
> >   Generator->write();
> > ```
> > 
> > the reader would also need a different constructor or a different type hierarchy and the work flow of llvm-profgen may not look as clean as now
> 
> It'd be a very thin wrapper over the actually llvm profile reader. Maybe a standalone function is all we need, instead of a new reader class. we take file as input, produced ProfileMap and pass it to generator. The use case and workflow is indeed special, it's like a side channel not tied to the main use of the tool (raw profile to llvm profile). Forcing that special workflow to use the same structure of the rest creates the problems. Yes, it's tradeoff, but shove llvm profile reader under the hierarchy of perf reader doesn't look like a good one to me. 
Changed to using a separate channel for sample profile input. 


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:53
+static cl::opt<std::string> SymbolizedProfFilename(
+    "symbolized-profile", cl::value_desc("symbolized profile"), cl::ZeroOrMore,
+    llvm::cl::MiscFlags::CommaSeparated,
----------------
hoy wrote:
> wenlei wrote:
> > This is just PGO profile right? If so, I don't think we need yet another name for it. We can say something like `llvm-pgo-profile` to be clear.
> > 
> > Do you intend to make this work for all kinds of pgo profiles as input? like CSSPGO/AutoFDO, flat vs nest etc. 
> llvm-pgo-profile sounds better. Yes, I'd like all kinds for sample profiles to be read and postpreocessed by the tool.
I ended up ing llvm-sample-profile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121655