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

Lei Wang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 12 11:03:48 PST 2021


wlei added inline comments.


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


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:47
+  PerfReader Reader(BinaryFilenames, PerfTraceFilenames);
   Reader.parsePerfTraces(PerfTraceFilenames);
 
----------------
wenlei wrote:
> 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. 
thanks for the discussing, fixed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96387



More information about the llvm-commits mailing list