[PATCH] D88714: [NewPM][CGSCC] Handle newly added functions not directly referenced by existing nodes

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:516
 
-  for (Node *NewNode : NewNodes)
+  while (!NewNodes.empty()) {
+    Node *NewNode = NewNodes.pop_back_val();
----------------
MaskRay wrote:
> The whole loop deserves a comment, along the line of: the newly created nodes are referenced by some SCC nodes via call/ref edges. We conservatively assume the newly created nodes are part of the SCC (as if they call back to the SCC).
Added comments, better?


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:518
+    Node *NewNode = NewNodes.pop_back_val();
+    if (NewNode->isPopulated())
+      continue;
----------------
MaskRay wrote:
> `assert(!NewNode->isPopulated())`
There might be multiple references to a new node added into `NewNodes` before we reach the new node.


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:522
 
+    SmallVector<Constant *, 16> NewNodesWorklist;
+    SmallPtrSet<Constant *, 16> NewNodesVisited;
----------------
MaskRay wrote:
> Having the container in the loop can lead to duplicate traversal. Move NewNodesWorklist & NewNodesVisited outside the loop (and decrease 16 -> 4 may be sufficient)
> 
> Additionally, should `NewNodesVisited` affect the G.getLibFunctions() loop below which uses `Visited`?
Each iteration will traverse a different function. The only potential speedup is if multiple functions reference the same `Constant`.

The `G.getLibFunctions()` part below is only relevant to the original function in `N`, doesn't affect the other functions we are traversing here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88714