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

FWIW, I think a lot of the issues here are at least much better understood. I've got a patch out that solves the most pressing of the invalidation problems within the framework of this design. I've got a devirtualization iteration utility posted. I've ported the inliner, and will mail that shortly -- i mostly was trying to test it a bit more heavily before sending it out, but I think that's counter-productive at this point.

There were a couple of specific comments on this patch outstanding that I've tried to address. Sorry for the delay there.

There are still several invalidation problems that need to be fixed. But I've started fixing them, and have the beginnings of patches, but I'm having a lot of trouble testing the fixes effectively. I'd like to do a fairly substantial refactoring of the unit tests, but I'd really rather do that *after* this lands so that I don't have to refactor the unittests twice essentially (all of the APIs will change with this patch).

Hoping this patch is in a state where folks can review again. I know the algorithms are pretty long, and there may even be bugs there that we'll have to fix as more testing lines up, but so far the testing of this particular part of the change seems to be working out fairly well -- I have way more bugs I need to fix in invalidation than in the graph update so far. =D


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:169
@@ +168,3 @@
+/// ignore this argument to their run method.
+struct CGSCCUpdateResult {
+  SmallPriorityWorklist<LazyCallGraph::RefSCC *, 1> &RCWorklist;
----------------
silvas wrote:
> Please add specific documentation about how this CGSCCUpdateResult is used by CGSCC passes to communicate the modifications. One possibility for "documentation" may be to add unit tests covering basic patterns like how to update them when removing a function, how to update them when replacing a function, how to update them when adding edges, etc.
> 
> But I would also expect at least some basic comments on each member explaining under what scenarios it needs to be used and how.
Yea, I had forgotten to do this for a long time because I kept changing what the members of this were. I never went back and documented them once things settled into a final form. Sorry about that.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:388
@@ -190,2 +387,3 @@
     FunctionAnalysisManager &FAM =
-        AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C).getManager();
+        AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, CG).getManager();
+
----------------
silvas wrote:
> This is error prone passing in both C and CG. For a given C there is only one valid CG and this signature gives the false impression that there are two degrees of freedom here. You already have the relevant back pointers to fetch the CG as needed so just use that.
Unfortunately, there aren't a lot of great alternatives.

This replaces an already error prone pattern where the pass would immediately grab the analysis by calling 'getCachedResult' and asserting that it got a non-null pointer back. By passing in a reference to the deeply fundamental analyses I think it makes it more clear to the caller that these analyses have to be provided.

We could package all of these arguments inside a struct (possibly with an up-pointer in the SCC, but possibly with some other struct that bundles them). But I'm not sure that will really be a net improvement. I expect that functions which give this parameter a name at all would want to unpack any such struct immediately.

It is perhaps a bit weird that this API is really designed to optimize for implementor, but in a sense it is. This API gets implemented for each pass, but called in only a very few places.

Still, I'll definitely add some comments here to help make it very clear what the expected relationship is between the arguments.


https://reviews.llvm.org/D21464