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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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);
----------------
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?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:404
   BranchSample BranchCounter;
+  std::map<uint64_t, uint64_t> AddressCounter;
 
----------------
Do we really need this? `RangeCounter` should cover addresses?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:550
+    SmallVector<StringRef, 32> Records;
+    Line.split(Records, " ", -1, false);
+    if (Records.size() < 2)
----------------
Nit: splitting once is enough, instead of unlimited times (-1)?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:573
+      // Skip the aggregated count
+      if (!TraceIt.getCurrentLine().getAsInteger(10, FrameAddr))
+        TraceIt.advance();
----------------
Why need this? Can the count be just skipped since it does not reflect a call stack sample and LBR sample?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108153