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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

Thanks for the detailed review. PTAL!



================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:624
+  Expected<ArrayRef<memprof::MemProfRecord>>
+  getMemProfRecord(uint64_t FuncNameHash);
+
----------------
davidxl wrote:
> single line?
clang-format puts it on a separate line since the length is 85 chars.


================
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;
+
----------------
davidxl wrote:
> Why not StringMap?
The memprof data is indexed via uint64_t obtained from llvm::md5(FunctionName). The instrprof data is first indexed by function name as string and then via the cfg hash thus uses a StringMap.


================
Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:66
 
+  void addRecord(const ::llvm::memprof::MemProfRecord &MR,
+                 function_ref<void(Error)> Warn);
----------------
davidxl wrote:
> Should there be a merge interface like the addRecord for Edge profile data above?
The existing mergeRecordsFromWriter function defined below has been extended to merge memprof data if necessary. See the changes in lib/ProfileData/InstrProfWriter.cpp:L287.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:57
+  // Write the contents of the MemInfoBlock based on the \p Schema provided to
+  // \p Ptr.
+  char *serialize(const MemProfSchema &Schema, char *Ptr) const {
----------------
tejohnson wrote:
> Note the return value?
I decided to rewrite this function to accept raw_ostream& OS instead of char *Ptr. This allowed me to remove a memory allocation TODO in a higher up callee. The function no longer returns a value.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:123
+  PACKED(struct Frame {
+    uint64_t Function;
     uint32_t LineOffset;
----------------
tejohnson wrote:
> davidxl wrote:
> > Document type -- MD5 Hash
> Can we use GlobalValue::GUID as the type (which is uint64_t)? That's already used elsewhere in this directory (at least in the SampleProfiler). If so, then the later md5 hash computation can call the static helper Function::getGUID on the string (which is just a wrapper around MD5Hash but matches the type name).
Documented the frame and all the members.

Changed the type to GlobalValue::GUID. While I don't expect the type to change, I added a static assert in the writer to ensure that the writer and reader code does not bitrot in case it does.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:231
+
+  uint64_t ReadKey(const unsigned char *D, offset_type) {
+    return *reinterpret_cast<const uint64_t *>(D);
----------------
tejohnson wrote:
> expected that second parameter unused?
Yes, added /*Unused*/ for everywhere this is applicable to make it clear.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:235
+
+  data_type ReadData(uint64_t K, const unsigned char *D, offset_type N) {
+    Records = deserializeRecords(Schema, D);
----------------
tejohnson wrote:
> ditto - will we get unused warnings about N?
No warnings for unused parameters AFAICT.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:263
+    if (!Frame.IsInlineFrame)
+      Key = Frame.Function;
+  }
----------------
tejohnson wrote:
> What is the intention of the code - this will overwrite Key for every non-inline frame. Should it break after Key is assigned the first time? Or do we want the last one, in which case you could walk the CallStack in reverse order and again break when Key is set for the first time.
It is the latter. I changed the loop to iterate in reverse and break early.


================
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.
+
----------------
davidxl wrote:
> Requires text format reader and writer for memprof
We have a text format writer, it prints it to YAML right now. The reader from YAML has not been implemented yet. This comment however alludes to the lack of the iterator interface to the memprof data. That requires a little bit of code which I wanted to keep separate to minimize the size of this patch.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:257
+    std::unique_ptr<RawMemProfReader> Reader = std::move(ReaderOrErr.get());
+    // Check if the profile types can be merged.
+    if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
----------------
tejohnson wrote:
> When would this fail?
For now, when clang frontend profiles are merged with memprof. Updated the comment to clarify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118653