[PATCH] D116784: [ProfileData] Read and symbolize raw memprof profiles.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:42
+      OS << "    -\n"
+         << "      function: " << Frame.Function << "\n"
+         << "      line_offset: " << Frame.LineOffset << "\n"
----------------
To handle deep stacks, perhaps make one line per frame (with indentation to show level)?


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:50
+
+    // TODO: Replace this once the format is updated to be version agnostic.
+#define PRINT(Field) OS << "      " #Field ": " << Info.Field << "\n"
----------------
This TODO needs more explanation.


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:33
 
+using CallStackMap = llvm::DenseMap<uint64_t, llvm::SmallVector<uint64_t, 32>>;
+
----------------
Document key and value in the map?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:92
+
+template <typename T> llvm::SmallVector<T, 16> readInfo(const char *Begin) {
+  const uint64_t NumItemsToRead = *reinterpret_cast<const uint64_t *>(Begin);
----------------
document the method.


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:52
+CHECK-NEXT:       function: {{[0-9]+}}
+CHECK-NEXT:       line_offset: 73
+CHECK-NEXT:       column: 3
----------------
Is this line correct? where is the source file to validate?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:70
+CHECK-NEXT:       dealloc_timestamp: 987
+CHECK-NEXT:       total_lifetime: 987
+CHECK-NEXT:       min_lifetime: 987
----------------
unit of the time?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:73
+CHECK-NEXT:       max_lifetime: 987
+CHECK-NEXT:       alloc_cpu_id: 4294967295
+CHECK-NEXT:       dealloc_cpu_id: 56
----------------
is cpuid this big?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116784