[PATCH] D89723: [CSSPGO][llvm-profgen]Context-sensitive profile data generation

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/Inputs/noinline-cs-noprobe.perfscript:2
+Using perf wrapper that supports hot-text. Try perf.real if you encounter any issues.
+PERF_RECORD_MMAP2 2854748/2854748: [0x400000(0x1000) @ 0 00:1d 123291722 526021]: r-xp /home/noinline-cs-noprobe.perfbin
+
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > I think we also need to support cases where PERF_RECORD_MMAP2 event isn't available, in which case we just use preferred load address from ELF header. 
> > > 
> > > Can you add a test case that doesn't have PERF_RECORD_MMAP2? Looks like currently we would just proceed with parsing without a base address set?
> > Yeah, currently PERF_RECORD_MMAP2 is required.
> > The problem using preferred load address for non-mmap event is one perf address might belong to multiple binaries, which will mess up the whole process. Also we need to one more perftrace scan to confirm there is no mmap2 event so that we can switch to use preferred address.
> > or we can have a switch like "--no-mmp2-events" to explicitly tell the tool use preferred address, also only support one binary under this switch. or we need some info in the perf trace tell which binary it belong to(I remembered we discuss this internally). any suggestion on this?
> Maybe the binary lookup table can be pre-filled with preferred load address when the binary is loaded/constructed. Without mmap2 events in the trace file, subsequent processing with just use the preferred addresses.
Yeah, what @hoy suggested is what I was thinking about - default to preferred load address if mmap is absent. We need that but I think It's fine to deal with it in a separate patch.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:156
+  const ProfiledBinary *getBinary() const { return Binary; }
+  bool hasNextLBR() { return LBRIndex < LBRStack.size(); }
+  uint64_t getCurrentLBRSource() const { return LBRStack[LBRIndex].Source; }
----------------
const qualifier here as well?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:224
+call, push frame on call stack when LBR is return. After each LBR processed, it
+also needs a linear unwinding to match the next LBR and over those unwinding
+process we will record each call stack as context id and LBR/linear range as
----------------
For linear unwinding, some brief explanation for handling of inlining would be helpful too. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:47
+void ProfileGenerator::write() {
+  auto WriterOrErr = SampleProfileWriter::create(OutputFilename, OutputFormat);
+  if (std::error_code EC = WriterOrErr.getError())
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > I'm wondering if a separate profile file should be output for each binary. Since the samples are already separated for binaries via `BinarySampleCounters`,  `ProfileMap` can be made like that too.
> > Yeah, it's doable. but that needs more CL design, currently we only support one output file, so we have to change supporting multiple output files which also need an exact one-one mapping to the binary. So we can use `OutputFilenames` to receives multiple output files and match them in order on the command line? or I'm also thinking we just remain this and if the user really need to separate the output for binary, they could call the tool multiple times with different input binary. any suggestions on the command?
> I see. Let's keep a single output for now.
What about limiting to single binary input for now? Error our with message saying unsupported if multiple binaries are provided. Generating profiles for multiple binaries in a single output file will make the profile summary info inaccurate (e.g. percentile based hot thresholds). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723