[PATCH] D118653: [memprof] Extend the index prof format to include memory profiles.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/InstrProfData.inc:665
  * The 61st bit indicates function entry instrumentation only.
+ * The 62nd bit indicates whether memory profile information is present.
  */
----------------
tejohnson wrote:
> I noticed this change and the one below are not in compiler-rt/include/profile/InstrProfData.inc, I assume these two files should be identical?
yes.


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:624
+  Expected<ArrayRef<memprof::MemProfRecord>>
+  getMemProfRecord(uint64_t FuncNameHash);
+
----------------
single line?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:44
+  // the md5 hash of the function name is used to index into the map.
+  memprof::FunctionMemProfMap MemProfData;
+
----------------
Why not StringMap?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:66
 
+  void addRecord(const ::llvm::memprof::MemProfRecord &MR,
+                 function_ref<void(Error)> Warn);
----------------
Should there be a merge interface like the addRecord for Edge profile data above?


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:123
+  PACKED(struct Frame {
+    uint64_t Function;
     uint32_t LineOffset;
----------------
Document type -- MD5 Hash


================
Comment at: llvm/test/tools/llvm-profdata/memprof-merge.test:39
+For now we only check the validity of the instrumented profile since we don't
+have a way to display the contents of the memprof indexed format yet.
+
----------------
Requires text format reader and writer for memprof


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118653