[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:
> 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();
```



================
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,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121655