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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> original non-modified profile

nit: rephrase to be clearer what it means, or just simplify "llvm pgo profile".



================
Comment at: llvm/test/tools/llvm-profgen/cold-profile-trimming-symbolized.test:2
+; RUN: llvm-profgen --format=text --unsymbolized-profile=%S/Inputs/cold-profile-trimming.raw.prof --binary=%S/Inputs/inline-noprobe2.perfbin --output=%t1 --use-offset=0
+; RUN: llvm-profgen --format=text --symbolized-profile=%t1 --binary=%S/Inputs/inline-noprobe2.perfbin --output=%t2 --trim-cold-profile=1 --profile-summary-cold-count=1000
+; RUN: FileCheck %s --input-file %t2 --check-prefix=CHECK-TRIM
----------------
Maybe we should probably support no-diff llvm profile to llvm profile conversion? You mentioned the call site samples as a difference and I'm wondering whether this is an easy to fix difference. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:68
   UnsymbolizedProfile = 3, // Unsymbolized profile generated by llvm-profgen.
-
+  SymbolizedProfile = 4,   // Symbolized profile generated by llvm-profgen.
 };
----------------
I'd prefer not to introduce a new term, given we already have a proper name for it - LLVM PGO profile. Also this is not really PerfFormat anyways. 

I think it's fine and probably clearer to create a side channel to deal with llvm profile as input, rather than trying to fit in the existing perf format/reader structure. Taking llvm profile as input is indeed a special mode and use of llvm-profgen anyways. 


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121655