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

Snehasish Kumar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 22 14:42:38 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list