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

Thu Jun 30 17:14:52 PDT 2022

snehasish added a comment.

Thanks for splitting out the util change and adding unit tests. It made it much easier to review!

================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:41
+// The stack metadata is the first operand of each memprof MIB metadata.
+static inline MDNode *getMIBStackNode(const MDNode *MIB) {
+  assert(MIB->getNumOperands() == 2);
----------------
I think defining static inline functions in a header will create an instance for each module this file is included in. I think this is a discouraged pattern. Should we move this and the other helper below to the cpp instead?

================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:98
+
+  bool empty() const { return Alloc == nullptr; }
+
----------------
This function is unused in this patch. Perhaps you meant to use it at L100? It would flip the if condition though.

================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:105
+  /// caller order).
+  void addCallStack(AllocationType AllocType, ArrayRef<uint64_t> StackIds);
+
----------------
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.

================
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).
----------------
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.

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:77
+static bool hasSingleAllocType(uint8_t AllocTypes) {
+  switch (AllocTypes) {
+  case static_cast<uint8_t>(AllocationType::Cold):
----------------
I think this switch can be replaced with popcount which is a little easier to follow.

```
const unsigned PopCount = countPopulation(AllocTypes);
assert(PopCount != 0);
return PopCount > 1;
```

https://github.com/llvm/llvm-project/blob/655bea4226b401a11164f99c6344e38d8742b8e4/llvm/include/llvm/Support/MathExtras.h#L567

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:128
+  assert(StackMD);
+  auto AllocType = getMIBAllocType(MIB);
+  std::vector<uint64_t> CallStack;
----------------
Can we just inline the getMIBAllocType call into L135 and avoid the single use AllocType var?

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

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:131
+  for (auto &MIBStackIter : StackMD->operands()) {
+    auto *Val = mdconst::dyn_extract<ConstantInt>(MIBStackIter);
+    assert(Val);
----------------
nit: A descriptive name like StackHash would make it easier to follow.

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:165
+  // so recursively descend into callers in trie.
+  if (Node->Callers.size()) {
+    bool NodeHasAmbiguousCallerContext = Node->Callers.size() > 1;
----------------
!Node->Callers.empty()

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:211
+  std::vector<Metadata *> MIBNodes;
+  assert(Alloc->Callers.size() > 0);
+  buildMIBNodes(Alloc, Ctx, MIBCallStack, MIBNodes,
----------------
Add a comment that `addCallstack` has not been called yet?
Also `!Alloc->Callers.empty()` since we just want to assert that Callers has some entries.

================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:215
+  // Should be left with only Alloc's location in stack.
+  assert(MIBCallStack.size() == 1);
+  CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
----------------
Consider inlining the comment into the assert, e.g. 
assert(MIBCallStack.size() == 1 && "Comment");

================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:174
+    ASSERT_EQ(StackMD->getNumOperands(), 2u);
+    auto *Val = mdconst::dyn_extract<ConstantInt>(StackMD->getOperand(0));
+    EXPECT_EQ(Val->getZExtValue(), 1u);
----------------
Calling this StackId would make the test easier to follow. Also applies for the other tests.

================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:175
+    auto *Val = mdconst::dyn_extract<ConstantInt>(StackMD->getOperand(0));
+    EXPECT_EQ(Val->getZExtValue(), 1u);
+    Val = mdconst::dyn_extract<ConstantInt>(StackMD->getOperand(1));
----------------
This should probably be ASSERT since if the first stackid isn't 1 then it doesn't make much sense to continue.

================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:262
+!1 = !{!2, !"cold"}
+!2 = !{i64 1, i64 2, i64 3, i64 3}
+!3 = !{!4}
----------------
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.

================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:299
+// context required to identify the allocation type.
+TEST_F(MemoryProfileInfoTest, ReTrimMIBContext) {
+  LLVMContext C;
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128854