[PATCH] D108153: [llvm-profgen] Support LBR only perf script

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> Unified to use show-raw-profile instead of show-unwinder-output to dump the intermediate raw profile, see the comments of the format of the raw profile. For CS profile, it remains to output the unwinder output

If these are considered actual profile (as the name `show-raw-profile` suggests), it's probably better to write to output file instead of dumping to stdout. How about just have a switch `skip-symbolization`, which writes the raw, unsymbolized profile to output file directly, and then skip all later processing.

The range representation between cs and non-cs profile is slightly different, perhaps they can also be unified.



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:706
+  for (const auto &Item : AggregatedSamples) {
+    const HybridSample *Sample = dyn_cast<HybridSample>(Item.first.getPtr());
+    computeCounterFromLBR(Sample, Item.second);
----------------
wlei wrote:
> hoy wrote:
> > I'm thinking of separating LBR samples from `HybridSample` and making `HybridSample` inherit that. This way they are technically separate and could reduce confusion. What do you think?
> Sounds good, changed!
I actually find the inheritance isn't really necessary now. The hashing specialization in LBRSample does not add value because the general version for HybridSample can handle that too, and a separate hash for LBRSample just adds duplication. On the contrary, how about we rename `HybridSample` to `PerfSample` and remove the inheritance structure all together?

Also I don't think HybridSample being subclass of LBRSample is good because this is more about composition. What about if we have a perf sample that only has call stack but not LBR? inheritance wouldn't work well for that (and we don't want virtual inheritance). This is similar to why we don't make subclass for aggregation count. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:550
+    SmallVector<StringRef, 32> Records;
+    Line.split(Records, " ", -1, false);
+    if (Records.size() < 2)
----------------
wlei wrote:
> hoy wrote:
> > Nit: splitting once is enough, instead of unlimited times (-1)?
>  -1(maximum number of times split) is the default value, here is for the 4th parameter which means to not keep the empty string, otherwise there are many empty record, it's not direct to get the first leading pointer. Seems C++ can't skip the middle default value.
`Line.split(Records, " ", 1, false);` instead?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:695
+        std::make_shared<StringBasedCtxKey>();
+    Key->Context = "empty";
+    Key->genHashCode();
----------------
Would an empty string (zero length) work? 


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:94
+  if (Reader->getPerfScriptType() == PERF_LBR)
+    return EXIT_SUCCESS;
+
----------------
As an intermediate state, print a warning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108153