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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
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);
----------------
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!


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:404
   BranchSample BranchCounter;
+  std::map<uint64_t, uint64_t> AddressCounter;
 
----------------
hoy wrote:
> Do we really need this? `RangeCounter` should cover addresses?
Good catch. I did check our internal tool, the addressCounter is actually used only without LBR sample, so here we indeed don't need this, thanks!


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:550
+    SmallVector<StringRef, 32> Records;
+    Line.split(Records, " ", -1, false);
+    if (Records.size() < 2)
----------------
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.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:573
+      // Skip the aggregated count
+      if (!TraceIt.getCurrentLine().getAsInteger(10, FrameAddr))
+        TraceIt.advance();
----------------
hoy wrote:
> Why need this? Can the count be just skipped since it does not reflect a call stack sample and LBR sample?
Here it's in fact to skip it. To distinguish LBR and Hybrid sample, it used `Count` to indicate call stack exists, >0 means hybrid sample. When adding the aggregated count, it will also increase `Count` and treat LBR sample as hybrid sample incorrectly. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108153