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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:522
+      if (!TraceIt.isAtEoF()) {
+        if (Count && TraceIt.getCurrentLine().startswith(" 0x"))
+          return true;
----------------
wenlei wrote:
> 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
> 
> 
>A comment showing expected input would be helpful. 
Example added

>For LBR part, do we want to check more than leading 0x?
Just separate a function for this and add another condition to check whether '/' exists

> but if samples don't have stack, we don't emit error when there's no lbr
I think I miss this. If there is no stack and no lbr, we should emit error earlier. Added in the latest diff, thanks for the remainder.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96387