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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:43
+  uint64_t Target;
+  // IsArtificial is marked true when current LBR address is from external code
+  // (such as dynamic libraries), see extractLBRStack.
----------------
An artificial branch stands for a series of consecutive branches starting from the current binary with a transition through external code and eventually landing back in the current binary. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:167-172
+  for (uint32_t I = 0; I < (uint32_t)Context1.size() - 1; I++) {
+    if (Context1[I].first != Context2[I].first ||
+        Context1[I].second != Context2[I].second)
+      return false;
+  }
+  return true;
----------------
Nit: `std::equal(Context1.begin(), Context1.begin() + Context1.size() - 1, Context2.begin(), Context2.begin() + Context2.size() - 1)`


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:198
+
+    if (I == ContextVec.size() - 1) {
+      // Only keep the function name for the leaf frame
----------------
`RemoveLeaf` should be checked here?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:361
+            if (PreOffset != 0) {
+              PrologEpilogSet.insert(PreOffset);
+            }
----------------
Please comment here and below that the prolog/epilog built here is based on an estimated size. Dwarf decoding is needed for a building precise prolog/epilog. 

Also how about separating the code to form a prolog/epilog builder that can be based on `FuncStartAddrMap`? The builder will be based on Dwarf CFI in the future.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:161
+
+  // Expand and get the context string of the current stack,
+  // It will search the disassembling info stored in AddrToLocMap
----------------
Nit: Get the full context string for the given call stack with inline context filled in?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723