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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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.
  */
----------------
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?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:621
 
+  /// Return the memprof recrods for the function identified by
+  /// llvm::md5(Name).
----------------
typo: recrods


================
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 {
----------------
Note the return value?


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:104
+#define MIBEntryDef(NameTag, Name, Type)                                       \
+  IsSame &= (Other.get##Name() == get##Name());
+#include "llvm/ProfileData/MIBEntryDef.inc"
----------------
You could just return false if they are unequal, then return true if we fall through, to avoid extra comparisons


================
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);
----------------
expected that second parameter unused?


================
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);
----------------
ditto - will we get unused warnings about N?


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:287
+
+  void EmitKey(raw_ostream &Out, key_type_ref K, offset_type) {
+    using namespace support;
----------------
expected that last parameter not used (here and in EmitData)?


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:1042
+    return make_error<InstrProfError>(instrprof_error::hash_mismatch,
+                                      "memprof record not found");
+  return *Iter;
----------------
Maybe include the hash value in the error?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:357
+  // Only write out all the fields except 'HashOffset' and 'MemProfOffset'. We
+  // need to remember the offset of these field to allow back patching later.
+  for (int I = 0; I < N - 2; I++)
----------------
"these fields"


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:369
+  uint64_t MemProfSectionOffset = OS.tell();
+  // Reserve the spec for MemProf table field to be patched later if this
+  // profile contains memory profile information.
----------------
s/spec/space/ ?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:263
+    if (!Frame.IsInlineFrame)
+      Key = Frame.Function;
+  }
----------------
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.


================
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())) {
----------------
When would this fail?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118653