[PATCH] D21464: [PM] WIP: Introduce basic update capabilities to the new PM's CGSCC pass manager, including both plumbing and logic to handle function pass updates.

[Sanjoy Das via llvm-commits]

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

Looks good overall, some minor comments inline.  Feel free to push after fixing (or address them post-commit).


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:42
@@ +41,3 @@
+/// FIXME: There is one major drawback of the reference graph: in its naive
+/// form it is quadratic because it represents denormalized indirect
+/// references. This can be fixed in a number of ways that essentially preserve
----------------
It wasn't clear what you meant by "denormalized indirect references".

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:160
@@ +159,3 @@
+/// on is mutating underneath it. In order to support that, there needs to be
+/// careful communication about the precise nature and rammifications of these
+/// updates to the pass management infrastructure.
----------------
s/rammifications/ramifications/

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:292
@@ +291,3 @@
+      assert(RCWorklist.empty() && "Should always start with an empty RefSCC worklist");
+      RCWorklist.insert(&InitialRC);
+
----------------
Why do you do this nested scheme instead of queueing all of `CG.postorder_ref_sccs()` into `RCWorklist`?  The answer is probably worth a comment.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:437
@@ -186,2 +436,3 @@
 
   /// \brief Runs the function pass across every function in the module.
+  PreservedAnalyses run(LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM,
----------------
Shouldn't this be "across every function in the SCC."?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:585
@@ -579,3 +584,3 @@
     /// TargetN. The newly added nodes are added *immediately* and contiguously
     /// prior to the TargetN SCC and so they may be iterated starting from
     /// there.
----------------
Document what you're returning?  Especially your assumption that the first node in the returned range is the SCC containing the source node.

Might be worth it to add a unit test for this new behavior (no need to block this patch on that though).

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:47
@@ +46,3 @@
+    if (DebugLogging)
+      dbgs() << "Running pass: " << Passes[Idx]->name() << " on " << *C << "\n";
+
----------------
Range `for`?

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:86
@@ +85,3 @@
+/// be a lambda because it would need to be a generic lambda.
+template <typename SCCRangeT>
+LazyCallGraph::SCC *processNewSCCRange(const SCCRangeT &NewSCCRange,
----------------
I understand this this is a local utility, but I still think it will be helpful to add one or two lines about what it does (or use a more descriptive verb than "process").

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:92
@@ +91,3 @@
+                                       CGSCCUpdateResult &UR,
+                                       bool DebugLogging = false) {
+  typedef LazyCallGraph::SCC SCC;
----------------
Not sure if the default for `DebugLogging` adds any value.

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:111
@@ +110,3 @@
+         "Cannot insert new SCCs without changing current SCC!");
+  C = &*NewSCCRange.begin();
+  assert(G.lookupSCC(N) == C && "Failed to update current SCC!");
----------------
This confused me for a moment -- how about calling the return value `NextC`?

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:145
@@ +144,3 @@
+  SmallSetVector<Function *, 4> PromotedCallTargets;
+  SmallSetVector<Function *, 4> DemotedRefTargets;
+  // First walk the function and handle all called functions. We do this first
----------------
I found these names a little off, I'd have called them `PromotedRefTargets` and `DemotedCallTargets` instead, since they're ref targets that were promoted and call targets that were demoted respectively.

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:199
@@ +198,3 @@
+    if (!RetainedEdges.count(&E.getFunction()))
+      DeadTargets.push_back({E.getNode(), E.isCall() ? Edge::Call : Edge::Ref});
+  for (auto DeadTarget : DeadTargets) {
----------------
Might be worth adding a `Edge::getKind` helper to use here.

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:240
@@ +239,3 @@
+      assert(G.lookupRefSCC(N) == RC && "Failed to update current RefSCC!");
+      for (RefSCC *NewRC : reverse(NewRefSCCs))
+        if (NewRC != RC) {
----------------
In `processNewSCCRange` you assume that the `SCC` containing `N` will be the first one in the post-order, but you don't assume the corresponding fact for `RefSCC` s here.  Why do we have this asymmetry?

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:244
@@ +243,3 @@
+          if (DebugLogging)
+            dbgs() << "Enqueing a new RefSCC in the update worklist: " << *NewRC
+                   << "\n";
----------------
s/Enqueing/Enqueuing/

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:307
@@ +306,3 @@
+
+      // Any analyses cached for this SCC are no longer valid as the shape has
+      // changed by introducing this cycle.
----------------
I'd s/valid/precise/ here (I can't imagine a real-world analyses that would be _incorrect_ after splitting out SCCs).

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:328
@@ +327,3 @@
+      if (DebugLogging)
+        dbgs() << "Enqueing the existing SCC in the worklist: " << *C << "\n";
+      // Enque in reverse order as we pop off the back of the worklist.
----------------
s/Enqueing/Enqueuing/

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:329
@@ +328,3 @@
+        dbgs() << "Enqueing the existing SCC in the worklist: " << *C << "\n";
+      // Enque in reverse order as we pop off the back of the worklist.
+      for (SCC &MovedC : reverse(make_range(RC->begin() + InitialSCCIndex,
----------------
Enqueue


https://reviews.llvm.org/D21464