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

Digging into some of these comments, will get David's in the next email...

In http://reviews.llvm.org/D21464#460784, @silvas wrote:

> Some initial comments.
>
> You've described a lot of implementation details (and implementation), but haven't described the final intended visitation behavior you are trying to implement (or why). For starters, what's wrong with the old PM CGSCC visitation order that requires doing something fundamentally different in the new PM?


I'll post an update to the patch that adds a much more detailed comment to the header to try and describe what this is trying to achieve at a high level, etc. It includes some of the why, but it doesn't going into very specific details. I'm happy to do so here and add the details you or others find sufficient relevant and not too brittle to go there.

To summarize what is wrong with the old visit order: it misses specific optimization opportunities, and makes it harder to reason about the call graph updates in a principled way. The latter becomes more important when there might be cached analyses attached to the call graph nodes. Consider if we wanted to lift some of the things in FunctionAttrs into an *analysis* over an SCC rather than an attribute. Now it is essential that update to the graph structure invalidate the right set of analyses.

As an example of the kind of missed optimizations, I'm going to add a test case where we successfully deduce readnone with the new pass manager but can't (no matter how many iterations we make) deduce it with the old one. This is because we don't refine the SCC graph structure, and do principled visitation to that graph structure. The examples in the test case are somewhat contrived in order to make good test cases, but they hopefully represent the general pattern that I'm worried about here: we are using the set of functions in the SCC to reason about it as an SCC but not actually updating that set of functions when the code changes.

> The general idea is clearly to maintain a bottom-up visitation behavior, but since this patch doesn't implement edge addition (which represents devirtualization and so is pretty important) the intended behavior there is unclear.


So, this *does* implement ref -> call promotion which is what I would expect devirtualization to look like in all cases. See below for details on your question.

The other case is for "out-of-thin-air" call (or ref) insertion such as instrumentation passes might do. Currently this isn't supported but could be added in the future.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:90
@@ +89,3 @@
+    for (Instruction &I : BB)
+      if (auto CS = CallSite(&I))
+        if (Function *Callee = CS.getCalledFunction())
----------------
(in case reading this against the current patch, this originally was attached to the FIXME regarding handling adding new calls)

As i somewhat alluded to above, what you describe should be handled by a ref edge turning into a call edge, and the update mechanism should be able to handle that well.

I've added a test case that exercises this with GVN and function-attrs. There is currently a case missed in the new pass manager because we don't have the up-to-four iteration whenever an indirect call turns into a direct call heuristic that the old pass manager has. I'm happy to add that, but I'd like to add it in a follow-up patch. I've marked where in the test case this is missed, and I've demonstrated that in theory this update mechanism is sufficient to handle it by explicitly running function-attrs again and it correctly catches the refinement.

The direct answer to your question is #2: it continues running on the now-larger SCC, detects that we switch from one SCC to another at some point, and re-runs on that SCC to make sure that the refined graph is observed.

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:170
@@ +169,3 @@
+
+    // The easy case is when the target RefSCC is not this RefSCC. This is
+    // only supported when the target RefSCC is a child of this RefSCC.
----------------
sanjoy wrote:
> Doesn't this also hold if the target SCC is not this SCC?
Yes, and inside the implementation of switchInternalEdgeToRef, it early exits with an empty range when we hit that scenario.

I can lift that distinction up into this code if you think that would be helpful, the somewhat arbitrary split was that these methods are on a RefSCC, and so it is the RefSCC-easy case that callers have to handle.


http://reviews.llvm.org/D21464