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

Fri Feb 24 15:38:00 PST 2023

tejohnson marked 3 inline comments as done.
tejohnson added a comment.

I'm about to update the patch with the version that uses shared_ptr for the edges. I'm also using unique_ptr to simplify management of ContextNodes, where I found some leaks happening while debugging my shared_ptr usage. Also made a change to print the context ids in sorted order to avoid spurious test case changes going forward, but that is going to result in some test changes with this update.

Patch not yet ready for re-review, I am working on addressing the other comments while merging in some fixes I made. I also have not yet updated the follow on patches to reflect the shared_ptr changes, will do that later.

================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:805
+    if (TowardsCallee) {
+      auto *NewEdge = new ContextEdge(Edge->Callee, NewNode,
+                                      computeAllocType(NewEdgeContextIds),
----------------
tejohnson wrote:
> 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...
I found and fixed a bug causing this blow up. The shared_ptr memory seems reasonable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908