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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:730
 
+class SymbolizedProfileReader : public PerfReaderBase {
+public:
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121655