[PATCH] D130717: [SCCIterator] Fix an issue in scc_member_iterator sorting

Mon Mar 27 09:24:37 PDT 2023

hoy added inline comments.

================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:346-349
+  // Starting from nodes that have no incoming edge. These nodes are "roots" of
+  // the MST forest. This ensures that nodes are visited before their
+  // decsendents are, thus ensures hot edges are processed before cold edges,
+  // based on how MST is computed.
----------------
wenlei wrote:
> nit: this comment seems slightly misplaced? It describes the initialization of the queue on line 353. And slightly duplicated with the comment on line 354. Maybe move there and merge the comments. 
The comment was supposed to explain a bit on the Kahn's algorithm. The next comment was mainly for getting a deterministic walk. I just tweaked them a little bit. Let me know if it looks good now.

================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:370
+      NodeInfoMap[Edge.Target].IncomingMSTEdges.erase(&Edge);
+      if (MSTEdges.count(&Edge) &&
+          NodeInfoMap[Edge.Target].IncomingMSTEdges.empty()) {
----------------
wenlei wrote:
> Do we need to check the visited flag still? If not, we can remove the flag?
Good point. It's used any more up since here but used when initializing the queue. I'm removing the setting of it in this loop.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130717