[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:522
+      if (!TraceIt.isAtEoF()) {
+        if (Count && TraceIt.getCurrentLine().startswith(" 0x"))
+          return true;
----------------
wlei wrote:
> wenlei wrote:
> > A comment showing expected input would be helpful. For LBR part, do we want to check more than leading 0x? 
> > 
> > Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?
> > Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?
> 
> Do you mean we should have three state for that?
> 1) LBR + call stack, which is detected in this function.
> 2) LBR only (when we detect the LBR  but `Count` is zero)
> 3) no LBRs at all.
> For the 3) case, we will emit an error.
> 
> 
Ok now I see the following error for #3, thanks for clarifying. Though I think it handles the case for hybrid sample, but if samples don't have stack, we don't emit error when there's no lbr? But that's probably fine as regular (non-hybrid) one is a TODO anyways.

> 'Hybrid perf sample is corrupted, No LBR sample line




================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:47
+  PerfReader Reader(BinaryFilenames, PerfTraceFilenames);
   Reader.parsePerfTraces(PerfTraceFilenames);
 
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96387