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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Just read the unwinding part and have some questions inlined. Could you add some standalone tests for unwinding part?



================
Comment at: llvm/test/tools/llvm-profgen/inline-cs-noprobe.test:1
+; RUN: llvm-profgen --perfscript=%S/Inputs/inline-cs-noprobe.perfscript --binary=%S/Inputs/inline-cs-noprobe.perfbin --output=%t
+; RUN: FileCheck %s --input-file %t
----------------
Is it possible to use a small manually crafted perfscript as input? It is easier to know whether the number in the output makes sense or not when the perfscript is small. It will also be easier if something in the test needs to be adjusted in the future.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:133
+      // Unwind branches - for regular intra function branches, we only
+      // need to record branch with context, push/pop for stack needed.
+      unwindBranchWithinFrame(State);
----------------
It is needed or unneeded? Without call/ret, I assume there is no need to push/pop callstack?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:104
+    uint64_t StackLeaf = CallStack.front();
+    if (StackLeaf < LBRLeaf || StackLeaf >= LBRLeaf + 0x100) {
+      WithColor::warning() << "Bogus trace: stack tip = "
----------------
What is the rationale behind the condition?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:126-127
+using BinaryAddressMap = std::map<uint64_t, ProfiledBinary *>;
+// A map of sample counter with context, this is used to aggregate and
+// accumulate sample count with the same context.
+using ContextToSampleMap =
----------------
What do the three uint64_t fields in the map represent?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:205
+
+  static bool checkHybridPerfScript(StringRef FileName) {
+    // Context-sensitive profile traces are seprated by double line break,
----------------
Rename it to 'isCtxSensitivePerfScript'?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723