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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks.



================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:155-156
+      // Replace existing call edges with same target but smaller weight.
+      Edges.erase(EdgeIt);
+      Edges.insert(Edge);
     }
----------------
Do you actually need to erase/insert, instead of updating the weight? 


================
Comment at: llvm/lib/Transforms/IPO/ProfiledCallGraph.cpp:138
+  for (auto *Node : InputNodes)
+    (void)NodeInfoMap[Node].Group;
+
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > 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.
> > I think what you need is `NodeInfoMap.emplace(Node, NodeInfo())`. See https://godbolt.org/z/Wrx61rYcY.
> That’ll construct a temp object whose `Groups` points to itself. The temp object will be used to copy construct the final object whose `Groups` will point to the temp object which isn’t what we want. Also this still cannot avoid a copy construction.
Ok, now I get what you're trying to do.. copy-constructor would be a move-constructor but the this pointer won't be updated still which is the trick you need. I guess it's fine with a comment. Thanks for clarification. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114204