[PATCH] D140908: [MemProf] Context disambiguation cloning pass [patch 1/3]

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:131
   CallStackIterator beginAfterSharedPrefix(CallStack &Other);
+  uint64_t last() const;
 
----------------
snehasish wrote:
> nit: call this back() to be consistent with iterator method names?
I split this out into D143184, and renamed last() to back() as suggested.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:805
+    if (TowardsCallee) {
+      auto *NewEdge = new ContextEdge(Edge->Callee, NewNode,
+                                      computeAllocType(NewEdgeContextIds),
----------------
tejohnson wrote:
> snehasish wrote:
> > Can we use shared_ptr for the edges so that we don't have to manually track ownership?
> > 
> > (also mentioned in the other patch)
> Yes we could. I have some concern about the additional overhead and whether it is worth it given that tracking these didn't end up being too difficult. I'll collect some measurements, it might be small given the overhead of all the rest of the graph memory.
I have been playing around with using shared_ptr for the ContextEdges. For some reason, the memory is really blowing up. I expect some increase, but not what I am seeing. I confirmed I am using make_shared which should minimize the overhead, and I don't see where I would be creating extra copies that live somewhere longer than expected, but I feel like I must be doing something wrong. Still looking...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908