[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 added a comment.

Replied to comments.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:92
@@ +91,3 @@
+                                       CGSCCUpdateResult &UR,
+                                       bool DebugLogging = false) {
+  typedef LazyCallGraph::SCC SCC;
----------------
chandlerc wrote:
> 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.
I meant passing in `false` for the default value of `DebugLogging`.  Looks like you explicitly pass in `DebugLogging` for both the places that call `processNewSCCRange` so the default isn't needed.

================
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!");
----------------
chandlerc wrote:
> 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.
I was reading this as a "transition function" that returns the new state of the algorithm (so it returns the "next" state).

Passing in `LazyCallGraph::SCC *&C` and updating `C` in place is fine too, in which case I'll read the function as "destructively modify these variables to reflect that we have a set of new `SCC` s".

However, this is not a big deal, and using `C` is fine 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) {
----------------
chandlerc wrote:
> 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?
A comment noting that this was intentional when written and not an oversight will be great. 


Repository:
  rL LLVM

https://reviews.llvm.org/D21464