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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
