[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[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/Transforms/Instrumentation/PGOInstrumentation.cpp:1276
+  if (Error E = MemProfResult.takeError()) {
+    handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
+      auto Err = IPE.get();
----------------
tejohnson wrote:
> snehasish wrote:
> > Consider defining the lambda outside above the condition to reduce indentation. IMO it will be  a little easier to follow if it wasn't inlined into the if statement itself.
> I could do this, but as is it mirrors the structure of the similar handling in readCounters, which has some advantages. wdyt?
I wasn't a big fan of the existing structure in readCounters but I didn't want to ask you to change the other code. Let's leave it as is for now. 


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1360
+      // If leaf was found in a map, iterators pointing to its location in both
+      // of the maps (it may only exist in one).
+      std::map<uint64_t, std::set<const AllocationInfo *>>::iterator
----------------
tejohnson wrote:
> snehasish wrote:
> > Can you add an assert for this?
> In this case "may only" meant "might only", not "may only at most". So I can't assert on anything. This can happen for example if we have a location that corresponds to both an allocation call and another callsite (I've seen this periodically, and can reproduce e.g. with a macro). We would need to use discriminators more widely to better distinguish them in that case (with the handling here we will only match to the allocation call for now - edit: a slight change noted further below ensures this is the case). Will change /may/might/ and add a note.
Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142