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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

I'll move on to the other patches in the stack and then come back to this one, thanks for your patience!



================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:915
+    // inlined calls to context node sequences we will sort the vectors of stack
+    // ids in reverse order of length, and within each length, lexicographically
+    // by stack id. The latter is so that we can specially handle calls that
----------------
nit: s/reverse/descending?


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:956
+        auto *Edge = Node->findEdgeFromCallee(PrevNode);
+        // If there is no edge then the nodes belong to different MIB contexts,
+        // and we should skip this inlined context sequence.
----------------
I didn't understand how we can have this case with different MIB contexts here. Can you elaborate in the comment?


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:977
+
+      ContextNode *LastNode = CurNode;
+
----------------
I think LastNode is redundant and CurNode can be used in L1016 and L1024. 


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1500
+    DenseSet<uint32_t> CallerEdgeContextIds(Edge->ContextIds);
+    for (EI++; EI != Node->CallerEdges.end(); EI++) {
+      auto *Edge = *EI;
----------------
Prefer incrementing this iterator outside the for loop. It was a little strange to see EI++ in the initialization and increment step.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1596
+template <typename DerivedCCG, typename FuncTy, typename CallTy>
+void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::exportToDot(
+    std::string Path) const {
----------------
It would be good to use llvm/Support/GraphWriter.h to export as dotGraph instead of re-implementing things here. You may need to use DOTGraphTraits.h too to implement some things such as tooltips which are used here.

This is probably a significant change but it would reduce the amount of code we have to maintain in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908