[PATCH] D128143: [MemProf] Update metadata during inlining

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:320
-/// allocates memory via new.
-bool llvm::isNewLikeFn(const Value *V, const TargetLibraryInfo *TLI) {
-  return getAllocationData(V, OpNewLike, TLI).hasValue();
----------------
I guess this was removed from the diff due to the rebase?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:805
+  }
+  return true;
+}
----------------
This will return true if one or both are empty. Do you want that to count as a common prefix?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:817
+static void updateMemprofMetadata(CallBase *CI,
+                                  std::vector<Metadata *> &MIBList) {
+  assert(!MIBList.empty());
----------------
const std::vector since it doesn't look like we modify the contents?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:823
+  CallStackTrie CallStack;
+  for (auto *MIB : MIBList)
+    CallStack.addCallStack(cast<MDNode>(MIB));
----------------
Spelling out the type here would be good. From the name I would have guessed this is a MemInfoBlock but its actually `Metadata` type.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:836
+static void
+propagateMemProfMetadata(const CallBase *OrigCall, CallBase *ClonedCall,
+                         MDNode *InlinedCallsiteMD,
----------------
Should we call this method propagateMemProfHelper or similar? As an overload it was a bit confusing to distinguish.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:881
+  }
+  if (!NewMIBList.empty()) {
+    if (NewMIBList.size() < OrigMemProfMD->getNumOperands()) {
----------------
I think if we check the else condition first we can return early and reduce the nesting here

```
if (NewMIBList.empty()) {
  removeMemProfMetaData
  removeCallsiteMetaData
  return
}
```


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:916
+  std::map<const CallBase *, std::vector<Metadata *>> OrigCallToNewMemProfMDMap;
+  for (auto Entry : VMap) {
+    auto *OrigCall = dyn_cast_or_null<CallBase>(Entry.first);
----------------
auto& to avoid copying the pair?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:919
+    auto *ClonedCall = dyn_cast_or_null<CallBase>(Entry.second);
+    if (!OrigCall || !ClonedCall)
+      continue;
----------------
Can you add a comment on why either of these could be null when cast to CallBase?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:936
+  // memprof MD on the inlined version of the call).
+  for (BasicBlock &BB : *Callee)
+    for (Instruction &I : BB) {
----------------
I think the guidance is to use braces in the outer block here since there are multiple levels of nesting.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/test/Transforms/Inline/memprof_inline.ll:1
+;; Test for memprof metadata propagation, ensuring metadata is simplified
+;; to function attributes appropriately after inlining profiled call chains.
----------------
Can you add the c++ code for memprof_inline.cc in the comments here? I think it might be useful to track which callsites we expect metadata on before reading through the verbose IR below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128143