[PATCH] D114204: [CSSPGO] Sorting nodes in a cycle of profiled call graph.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:48
+      if (L.Weight == R.Weight)
+        return L.Target->Name < R.Target->Name;
+      return L.Weight > R.Weight;
----------------
Can target still be the same, in which case do we want to check source name?


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:108
 
+class ProfiledCallGraphNodeSorter {
+  struct NodeInfo {
----------------
This is only for sorting SCC nodes in an SCC and it doesn't order all nodes in CG, so perhaps it's better to call it SCCNodeSorter. This also feels like a generic functionality that doesn't tie to ProfiledCallGraph, so we can perhaps make it a generic template similar to scc_iterator, e.g. scc_member_iterator.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:127
+
+void ProfiledCallGraphNodeSorter::sort(
+    const std::vector<ProfiledCallGraphNode *> &InputNodes,
----------------
This is not trivial, some comment explain 1) prerequisite and scope, i.e. within scc for scc members; 2) the high level objective of the sorting, i.e. visit hot edges first 3) the implementation and how that is guaranteed would be helpful. 

Either comment on the sort function or the class definition.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:138
+  for (auto *Node : InputNodes)
+    (void)NodeInfoMap[Node].Group;
+
----------------
nit: make this an explicit insert for readability


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:141-146
+  struct ProfiledCallGraphEdgeComparer {
+    bool operator()(const ProfiledCallGraphEdge *L,
+                    const ProfiledCallGraphEdge *R) const {
+      return L->Weight > R->Weight;
+    }
+  };
----------------
There's another ProfiledCallGraphEdgeComparer defined inside ProfiledCallGraphNode and that one also has tie breaker for equal weights. Perhaps we need a single global one? 


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:157
+
+  // Traverse all the edges and compute the Minimum Weight Spanning Tree
+  // using union-find algorithm.
----------------
> compute the Minimum Weight Spanning Tree

I think you meant *Maximum* weight spanning tree.

> using union-find algorithm.

on a high level this is Kruskal's algorithm, union-find is just one way to detect cycles.     


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:172
+
+  // Do BFS on MST, starting from nodes that have no incoming edge.
+  for (const auto *Edge : MSTEdges)
----------------
Comment on how this makes sure we visit hot edges first



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114204