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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
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();
----------------
snehasish wrote:
> I guess this was removed from the diff due to the rebase?
Yeah, hmm, it looks like this was here due to a bad rebase (notice there is another copy of this 2 functions up).


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:805
+  }
+  return true;
+}
----------------
snehasish wrote:
> This will return true if one or both are empty. Do you want that to count as a common prefix?
The assumption is that neither should be empty. Added an assert.


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


================
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.
----------------
snehasish wrote:
> 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.
Done here and for memprof_inline2.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128143