[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.

[Chandler Carruth via llvm-commits]

chandlerc marked 12 inline comments as done.
chandlerc added a comment.

Thanks so much Sanjoy! I've tried to essentially address all of these (with a couple of exceptions where I'd like further guidance on how to best address them in follow-up commits below).

Landing now, and will be preparing some follow-up patches and the next significant chunk of functionality for review.


================
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
----------------
sanjoy wrote:
> It wasn't clear what you meant by "denormalized indirect references".
Sure, I've spelled this out better in the comment, just let me know if it still isn't sufficietnly clear.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:292
@@ +291,3 @@
+      assert(RCWorklist.empty() && "Should always start with an empty RefSCC worklist");
+      RCWorklist.insert(&InitialRC);
+
----------------
sanjoy wrote:
> 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.
Commented. For easy discovery, the crux is that `CG.postorder_ref_sccs()` is lazy, and we want to be as lazy as we can be. The worklist exists for when we discover new `RefSCC`s during transformations.

================
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.
----------------
sanjoy wrote:
> 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).
Gah, thought I had done that. Thanks for spotting it.

Documentation updated, and yea I'll get this covered by a unit test. (It *is* covered by the pass manager tests as well, but a unit test would be really good here.)

================
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,
----------------
sanjoy wrote:
> 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").
Indeed, i've already written code that wanted to use it again. I've both tried to give it a better name and improved the comments.

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:92
@@ +91,3 @@
+                                       CGSCCUpdateResult &UR,
+                                       bool DebugLogging = false) {
+  typedef LazyCallGraph::SCC SCC;
----------------
sanjoy wrote:
> Not sure if the default for `DebugLogging` adds any value.
I don't think it does either. I'd really like to move all of the logging to use DEBUG and the DEBUG_PASS infrastructure. But I didn't want to do that in this change and the code already has the DebugLogging threaded through really pervasively.

================
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!");
----------------
sanjoy wrote:
> This confused me for a moment -- how about calling the return value `NextC`?
Hmm, but it's not the next C... its the "current" C. Or the component containing N.

Let's keep chatting to see if there is a better name, and I'll fix this up with whatever we come up with.

================
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
----------------
sanjoy wrote:
> 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.
Agreed.

================
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) {
----------------
sanjoy wrote:
> 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?
My memory is because of the nature of how we split SCCs vs. how we split RefSCCs. For the former we use the existing postorder sequence to constrain the precise returned order. For the RefSCCs, while the order is deterministic, it isn't predictable in the same way, and so we might have sibling RefSCCs and the node might end up in any particular one.

I think this would be worth fixing so that we can make a symmetric assumption. It may even already happen to be true. But I would want to spend some considerable time re-examining the RefSCC splitting algorithm to ensure this is by design and actually something we can guarantee. And we'd need a bunch of testing for it.

So, maybe this just needs a FIXME or a comment. What do you think? Where would you put them?


https://reviews.llvm.org/D21464