[PATCH] D94110: [CSSPGO][llvm-profgen] Aggregate samples on call frame trie to speed up profile generation

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:106
+                                         SmallVectorImpl<uint64_t> &CallStack) {
+  if (Cur->Address != 0) {
+    if (Binary->usePseudoProbes()) {
----------------
Is the trie root the only case this can be zero? Can we make an assert for that?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:112
+      // Cutting off the context from here since the inline will
+      // not know how to consume a context with unknown callsites.
+      if (!CallProbe) {
----------------
Typo: inliner


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:175
+  if (Binary->usePseudoProbes()) {
+    State.StackLeaf->Parent->recordBranchCount(Branch.Source, Branch.Target,
+                                               Repeat);
----------------
Would be nice to a comment about why counts are reported on the parent frame for pseudo prob .


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:213
+    Frame(uint64_t Addr) : Address(Addr) {}
+    Frame *getOrCreateFrame(uint64_t Address) {
+      auto Ret = Children.emplace(Address, std::make_unique<Frame>(Address));
----------------
Nit: how about name it `getOrCreateChildFrame`?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:226
+
+  Frame FrameTrieRoot;
+  Frame *StackLeaf;
----------------
`FrameTrieRoot` doesn't seem to be initialized anywhere in the constructor of `UnwindState`.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:227
+  Frame FrameTrieRoot;
+  Frame *StackLeaf;
   // Used to fall through the LBR stack
----------------
Nit: name it `FrameCurrentLeaf`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94110