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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 3 inline comments as done.
tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:1
+//===- llvm/Analysis/MemoryProfileInfo.h - memory profile info ---*- C++ -*-==//
+//
----------------
snehasish wrote:
> 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
See D128854. I'll try to rebase this and the follow on inliner patch on top of that when I get a chance.


================
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.
----------------
snehasish wrote:
> Should this be a doxygen comment block with `///`?
Fixed in new patch.


================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:19
+
+#define DEBUG_TYPE "memory-profile-info"
+
----------------
snehasish wrote:
> 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-*".
The clang option is -fmemory-profile for instrumentation, so I've used that some places (e.g. the file names too) for clarity. MemProf is a nice short hand and used in the metadata. I don't have a strong opinion about which name should be used where. I've kept this as is for now in the new patch, let me know what your thoughts are on what is clearer.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142