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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/inline-cs-noprobe.test:27
+; original code:
+#include <stdio.h>
+
----------------
Can you please add a comment on what compiler command line switches are used to build the source code?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:49
+      uint64_t Start = PrevIP;
+      recordRangeCount(Start, End, State, Repeat);
+      End = IP.Address;
----------------
Nit: just use `PrevIP` here instead of using `Start`? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:226
+
+  if (ShowUnwinderOutput) {
+    printUnwinderOutput();
----------------
Nit: curly braces not needed for single-statement block.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:388
+    // LBR sample is encoded in single line after stack sample
+    llvm_unreachable("'Hybrid perf sample is corrupted, No LBR sample line");
+  }
----------------
Use `exitWithError`?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:83
+  // Call stack recorded in FILO(leaf to root) order
+  std::list<uint64_t> CallStack;
+  // LBR stack recorded in FIFO order
----------------
Nit: consider using `std::vector` to reduce the number of memory allocations and for better locality. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:155
+  }
+  ProfiledBinary *getBinary() { return Binary; }
+  bool hasNextLBR() { return LBRIndex < LBRStack.size(); }
----------------
Nit: `const` qualifier for these getters?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:309
+  void parsePerfTraces(cl::list<std::string> &PerfTraceFilenames);
+  AggregationCounter &getSamples() { return AggregatedSamples; }
+  AddressBinaryMap &getAddrToBinaryMap() { return AddrToBinaryMap; };
----------------
Nit: `const` qualifier for getters?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723