[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

I'm still going through PGOInstrumentation.cpp ...



================
Comment at: clang/test/CodeGen/memprof.cpp:15
+// # Collect memory profile:
+// $ clang++ -fuse-ld=lld -Wl,-no-pie -Wl,--no-rosegment -gmlt \
+//      -fdebug-info-for-profiling -mno-omit-leaf-frame-pointer \
----------------
Just `-no-pie` is better (details: https://reviews.llvm.org/rG103b28902fd6)


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:1
+//===- llvm/Analysis/MemoryProfileInfo.h - memory profile info ---*- C++ -*-==//
+//
----------------
Can you split out the memory profile info parts into a separate patch? Also I think the interface is simple enough to be able to unit test. Something set up like [1] would be great. Then we can call addCallstack with different annotations followed by buildAndAttachMIBMetaData followed by checking the metadata annotated on the call inst(s). What do you think?

[1] https://github.com/llvm/llvm-project/blob/3f8e4169c1c390fd086658c1e51983ee61bff9bc/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp#L71


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:40
+
+// Class to build a trie of call stack contexts for a particular profiled
+// allocation call, along with their associated allocation types.
----------------
Should this be a doxygen comment block with `///`?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:333
 
+  bool hasMemoryProfile() const override {
+    return (Version & VARIANT_MASK_MEMPROF) != 0;
----------------
The RawInstrProfReader shouldn't have the memprof mask set. We have a separate  raw binary format which is independent. So this should always return false. Also maybe add a comment to document the fact?


================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:19
+
+#define DEBUG_TYPE "memory-profile-info"
+
----------------
We use MemoryProfile and MemProf interchangeably. Does it make sense to pick one and make it consistent throughout? Here for eg. the flags begin with "memprof-*" but the debug type is "memory-profile-*".


================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:102
+        assert(AllocStackId == StackId);
+        Alloc->AllocTypes |= (uint8_t)AllocType;
+      } else {
----------------
nit: prefer static_cast<uint8_t> here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142