[PATCH] D96387: [CSSPGO][llvm-profgen] Renovate perfscript check and command line input validation

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:63
+  PERF_UNKNOWN = -1,
   PERF_INVILID = 0,
   PERF_LBR = 1,       // Only LBR sample
----------------
typo: PERF_INVALID. Can we keep the enum 0 based?




================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:47
+  PerfReader Reader(BinaryFilenames, PerfTraceFilenames);
   Reader.parsePerfTraces(PerfTraceFilenames);
 
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > We should probably return here with `--show-disassembly` and the output file should have the disassemblies instead of a sample profile.
> > > I thought `show-disassembly` prints the assembly to stdout, while the generated profile goes into output file. However, if we want the tool to show assembly instead of generating profile, we may need to rename the switch to something like `show-disassembly-only`.
> > I see, how about we support both?
> > `show-disassembly` print to stdout and also generate profile. 
> > `show-disassembly-only` only print to output file, this can be used for the disassembly output only test.
> Sounds good, thanks! 
These wrappers and layers in reader and binary feels like we're doing a bit too much for showAssembly. In the end, showAssembly is not the main purpose of the tool. How about simplify it:

- Let's always print showAssembly to stdout. This is more like a verbose log rather than output, so I think it's better to avoid dumping it in the output file.
- Only have `show-disassembly-only` mode (which is mostly used in combination with probe annotation), add early exit for that, and produce empty output profile. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96387