<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 7, 2016 at 7:08 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added a comment.<br>
<br>
Mostly a high-level comment below on the core of this patch's approach.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/CGSCCPassManager.h:111-123<br>
@@ -100,18 +110,15 @@<br>
     PreservedAnalyses PA = PreservedAnalyses::all();<br>
-    for (LazyCallGraph::RefSCC &RC : CG.postorder_ref_sccs()) {<br>
-      if (DebugLogging)<br>
-        dbgs() << "Running an SCC pass across the RefSCC: " << RC << "\n";<br>
+    std::vector<std::unique_ptr<CallGraphSCC>> SCCs;<br>
+    while (!CGI.isAtEnd()) {<br>
+      const std::vector<CallGraphNode *> &NodeVec = *CGI;<br>
+      SCCs.emplace_back(llvm::make_unique<CallGraphSCC>(CG, &CGI, NodeVec));<br>
+      // Copy the current SCC and increment past it so that the pass can hack<br>
+      // on the SCC if it wants to without invalidating our iterator.<br>
+      ++CGI;<br>
+      CallGraphSCC &CurSCC = *SCCs.back();<br>
+      for (int i = 0; i < (int)MaxCGSCCIterations; i++) {<br>
<br>
-      for (LazyCallGraph::SCC &C : RC) {<br>
-        PreservedAnalyses PassPA = Pass.run(C, CGAM);<br>
+        PreservedAnalyses PassPA = Pass.run(CurSCC, CGAM);<br>
<br>
-        // We know that the CGSCC pass couldn't have invalidated any other<br>
-        // SCC's analyses (that's the contract of a CGSCC pass), so<br>
-        // directly handle the CGSCC analysis manager's invalidation here. We<br>
-        // also update the preserved set of analyses to reflect that invalidated<br>
-        // analyses are now safe to preserve.<br>
-        // FIXME: This isn't quite correct. We need to handle the case where the<br>
-        // pass updated the CG, particularly some child of the current SCC, and<br>
-        // invalidate its analyses.<br>
-        PassPA = CGAM.invalidate(C, std::move(PassPA));<br>
+        PassPA = CGAM.invalidate(CurSCC, std::move(PassPA));<br>
<br>
----------------<br>
This is the core of this change, and it highlights the problem I have with this design. But I want to emphasize something: I freely admit that you can fix the immediate failure modes. I'll even suggest how you could do that. I still think that it is a problematic design though, and hopefully this helps surface why.<br>
<br>
So, the key thing here is that you are building "stable" SCCs with identity on the fly by allocating CallGraphSCC objects in the vector of unique pointers. You then use their identity to thread the analysis management layers. Notably, you use it to access the FunctionAnalysisManager via the CGSCCAnalysisManager via the ModuleAnalysis manager.<br>
<br>
Both the lookup below of the FunctionAnalysisManager's proxy and the invalidate here use this identity.<br>
<br>
However, if you were to run multiple CGSCC pass managers one after the other, they would build different SCCs each time. There is no way to persist this, at the end of the CGSCC pass manager run, the SCCs are going away.<br>
<br>
To make this code correct, you should probably use the 'clear' method on the CGSCCAnalysisManager at the end of the CGSCC pass manager run. I think there are some bugs in the current pass manager code you'll hit if you do that. We can write some unit tests that hit these (sorry, I missed them when originally working on it) and fix them. Once fixed, that should make this approach correct AFAICT. However, I don't like some of the consequences.<br>
<br>
If we fix the bugs in the 'clear' code path in the new pass manager, this will have the effect of invalidating all function analyses computed thus far (assuming it follows the current design direction of the pass manager thus far). And one of the really big goals of the new pass manager design was to reduce the re-computing of analyses through caching.<br>
<br>
The way I see to continue to get caching for function analyses with this approach would be to avoid doing downward invalidation[1]. That is, when an SCC pass invalidates SCC analyses, don't have that trigger invalidation of function analyses. We could probably make that direction work, but it would mean manually tracking function analysis invalidation here and in the module pass manager rather than relying on the wiring between the proxies to do this for us.<br>
<br>
So that is what I think it would look like to make the approach in this patch at least functional for the passes as they exist in the tree today.<br>
<br>
<br>
Beyond that, as Sean has said, this doesn't really have a good mechanism to support SCC-based analyses. They will currently work, but they will be cached against an SCC that isn't being updated, and they won't survive the CGSCC pass manager run or be queryable. Notably, if we want an SCC analysis, we won't be able to query it from a module pass without duplicating the logic here to run the SCC iterator over the module and produce SCCs that we can then query the CGAM with.<br>
<br>
Now, we have no need of that today. But I think it is a mistake to design in a different limitation like this.<br>
<br>
<br>
I understand that your goal in this Sean is to make incremental progress. However, I am personally much more interested in figuring out the long term design for the CGSCC passes and the new pass manager. I think it will create churn to maintain all of the different permutations, as I think even with this approach we will have significant differences and challenges with the new pass manager. And so I think we will end up supporting both the legacy pass manager, and this one, *and* the one with stable SCCs that provides a more direct solution to the analysis challenges outlined above.<br></blockquote><div><br></div><div><br></div><div>IIUC, whatever is missing in this patch  (e.g. survival across CGSCC PM run) is by-design. The approach is intended to be an intermediate solution with the goal to *minimize* functional changes and possible churns. As you noted, even with this approach we will have significant differences and challenges -- that is the exactly the reason why Sean's approach is very valuable to the transition.  We only need to make sure this approach does not miss out any functionality existing in the old PM.</div><div><br></div><div>Once the longer term solution you are pursuing is ready, I expect this change will go away, so I don't understand why you think it creates churns. In fact, I expect much less churn with this approach, and it also buys your more time to refine your design -- it is a win-win for everybody.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
[1]: "downward invalidation" is something I had planned to add, but hadn't gotten to yet. It clearly would benefit from being visible in the code though, so I'm going to work on a short patch that adds it and unit tests that show how it would work. Naturally, we can always go a different direction if needed.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D21921" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21921</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>