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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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;
----------------
wenlei wrote:
> Can target still be the same, in which case do we want to check source name?
Edges to be compared are from same caller, so the source should be the same. Call targets can also be the same since a caller may have different callsites to the same callee. The current model allows such edges with same callee but different weights to coexist. I think we should optimize it to just keep the edge with largest weight. 


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:108
 
+class ProfiledCallGraphNodeSorter {
+  struct NodeInfo {
----------------
wenlei wrote:
> 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.
A generic template class sounds better. That will require the `GraphTraits` definition for nodes to come with a weighted edge list. Nodes with that can use this sorter.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:127
+
+void ProfiledCallGraphNodeSorter::sort(
+    const std::vector<ProfiledCallGraphNode *> &InputNodes,
----------------
wenlei wrote:
> 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 added.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:138
+  for (auto *Node : InputNodes)
+    (void)NodeInfoMap[Node].Group;
+
----------------
wenlei wrote:
> nit: make this an explicit insert for readability
This is specifically used to construct a `NodeInfo` object in place. An insert operation will involve a copy construction which invalidate the initial value of the `Group` field which should be `this`. An emplace operation may be able to construct the object in place but I haven't figured out how to pass a none parameter into to.



================
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;
+    }
+  };
----------------
wenlei wrote:
> There's another ProfiledCallGraphEdgeComparer defined inside ProfiledCallGraphNode and that one also has tie breaker for equal weights. Perhaps we need a single global one? 
This one is used to sort edges. That one is used to deduplicate edges, ie. two callsites to same target with same weight are deduplicated, thus is more expansive.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:157
+
+  // Traverse all the edges and compute the Minimum Weight Spanning Tree
+  // using union-find algorithm.
----------------
wenlei wrote:
> > 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.     
Maximum Weight Spanning tree is more accurate.


================
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)
----------------
wenlei wrote:
> Comment on how this makes sure we visit hot edges first
> 
Comment added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114204