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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I've now addressed all the feedback except the following 2 items that I will do next, as these will cause more global churn:

- Use GraphWriter (I suspect this will cause some tests that check the graph output to need updating).
- Rename PGHO* to MemProf*



================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:872
+  DenseSet<ContextNode *> NodesToDelete;
+  auto ProcessNode = [&](ContextNode *Node,
+                         DenseSet<const ContextNode *> &Visited,
----------------
snehasish wrote:
> This lambda is doing a lot of work, can me move this out to its own method? It's a little hard to read right now because of the size and doing so would make it a little easier to read. 
Refactored out and now named assignStackNodesPostOrder


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:977
+
+      ContextNode *LastNode = CurNode;
+
----------------
snehasish wrote:
> I think LastNode is redundant and CurNode can be used in L1016 and L1024. 
We could, but it is clearer IMO to use a name that corresponds to the meaning at those uses below. In any case, this code has been changed and we now assign LastNode more directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908