[PATCH] D93828: [CGSCC][Coroutine][NewPM] Properly support function splitting/outlining

Xun Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 29 22:11:58 PST 2020


lxfind added a comment.

Thank you for working on this! LGTM. I will let others who are more familiar with SCC to accept it.
A few nits commented inline.

On a side note, I was somewhat surprised how little interface RefSCC provides, and how much heavy-weight lifting needs to be done by accessing data directly. Not necessarily in this patch, but adding a few more helper methods to RefSCC seems useful.
For example, I imagine having a method in RefSCC like this could be useful:

  SCC *RefSCC::addNewSCCWithSingleNode(Node* N, int Index);

which creates a new SCC with a single Node N and insert it into index Index in the SCC list of the RefSCC. I can see this method used 3 times by this patch.
Another is to add a version of LazyCallGraph::initNode that only takes one parameter which is Node (and having the original one calling it).



================
Comment at: llvm/lib/Analysis/LazyCallGraph.cpp:1625
+
+  for (Instruction &I : instructions(OriginalFunction)) {
+    if (auto *CB = dyn_cast<CallBase>(&I)) {
----------------
Would it be better to implement getEdgeKind() by iterating through the list of edges from the Node that corresponds to OriginalFunction? (that way you can also assert on the Ref Edge in release mode without extra overhead)


================
Comment at: llvm/lib/Analysis/LazyCallGraph.cpp:1684
+      // it is in the same SCC (and RefSCC) as the original function.
+      NewC = lookupSCC(OriginalN);
+      NewC->Nodes.push_back(&NewN);
----------------
NewC = OriginalC;


================
Comment at: llvm/lib/Analysis/LazyCallGraph.cpp:1697
+        // function but a new SCC.
+        RefSCC *NewRC = lookupRefSCC(OriginalN);
+        NewC = createSCC(*NewRC, SmallVector<Node *, 1>({&NewN}));
----------------
RefSCC *NewRC = OriginalRC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93828



More information about the llvm-commits mailing list