[PATCH] D128854: [MemProf] Add memprof metadata related analysis utilities

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:98
+
+  bool empty() const { return Alloc == nullptr; }
+
----------------
snehasish wrote:
> This function is unused in this patch. Perhaps you meant to use it at L100? It would flip the if condition though.
It's used in a follow on. I think it is more natural to use the Alloc variable directly there on L100. So instead I added a couple calls, before and after calling addCallStack on a new Trie, to invoke and test it.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:105
+  /// caller order).
+  void addCallStack(AllocationType AllocType, ArrayRef<uint64_t> StackIds);
+
----------------
snehasish wrote:
> I think this function is public for testing only and callers should prefer using the interface `addCallStack(MDNode *MIB);`? If so can you document this in the comment.
No, it is used during matching in D128142.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:112
+  /// Build and attach the minimal necessary MIB metadata. If the alloc has a
+  /// single allocation type, add a function attribute instead. Returns true if
+  /// memprof metadata attached, false if not (attribute added).
----------------
snehasish wrote:
> Can you elaborate on the rationale for function attributes for single allocation types? It does look like its simpler to access in the code. Is it also because it reduces overhead? If so can you document the rationale here.
Yes it is lighter weight and simpler to communicate to the lib call handling. It is the way the cloning code I've been testing tags allocation calls once we create allocation call clones with single allocation types. Added comment.


================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:129
+  auto AllocType = getMIBAllocType(MIB);
+  std::vector<uint64_t> CallStack;
+  for (auto &MIBStackIter : StackMD->operands()) {
----------------
snehasish wrote:
> Prefer a smallvector or maybe add CallStack.reserve since we know the number of operands?
Decided to use reserve since we know the number of entries we'll need.


================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:262
+!1 = !{!2, !"cold"}
+!2 = !{i64 1, i64 2, i64 3, i64 3}
+!3 = !{!4}
----------------
snehasish wrote:
> I guess having a repeated stack id is feasible with recursive calls during profiling. It doesn't matter much since its adding a function attribute in this case.
I think this is a typo in the test. In the follow on matching patch we collapse recursion. I've removed it here for simplicity.


================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:299
+// context required to identify the allocation type.
+TEST_F(MemoryProfileInfoTest, ReTrimMIBContext) {
+  LLVMContext C;
----------------
snehasish wrote:
> I think this test is redundant since at this point we have already tested
> * the ability to trim contexts in `TrimmedMIBContext`
> * the ability to extract the metadata node and call the underlying addCallStack() correctly in `SimplifyMIBToAttribute`.
> 
> Up to you if you want to keep it but I'm slightly in favour of removing this test if it doesn't cover any new logic.
I wanted to have a test that put these together since is functionality that will be used to update after inlining. I agree the components are tested separately already, but I kind of like having this for completeness. If you prefer though I can remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128854