[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:
> hoy wrote:
> > 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. 
> Ok, then add a comment please so others can understand why only weight is used and tier breaker is not needed. 
comment added.


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:138
+  for (auto *Node : InputNodes)
+    (void)NodeInfoMap[Node].Group;
+
----------------
wenlei wrote:
> hoy wrote:
> > 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.
> > 
> I thought `emplace()` without a parameter should just work, no? but yeah, I meant emplace.
No, neither `NodeInfoMap.emplace(Node);` nor `NodeInfoMap.emplace(Node, {});` works.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:177
+cl::opt<bool>
+    SortProfiledSCC("sort-profiled-scc", cl::init(true), cl::Hidden,
+                    cl::desc("Sort profiled recursion by edge weights."));
----------------
wenlei wrote:
> nit: sort-profiled-scc-member?
fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114204