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

================
Comment at: lib/Analysis/CGSCCPassManager.cpp:327
@@ -24,1 +326,2 @@
+  return *C;
 }
----------------
>> 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.
> 
> Please add test cases exhibiting this.

Er, the next sentence in what I wrote was:

>> I've added a test case that exercises this with GVN and function-attrs.

I think the updated patch has this test case in it. It is cgscc-observe-devirt.ll

>> 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.
> 
> Okay, please add a test case for that.

This is the same thing, and covered by the same test case. I was just trying to make sure I directly answer your question as well...


> Also add a test case demonstrating that we don't go quadratic on a graph like http://reviews.llvm.org/F2110388 where function passes manage to devirtualize all the ref edges.

I'm not really sure what you want here... In general it is very hard to have a test case in the regression test suite that demonstrates a lack of quadratic behavior -- it typically requires an unacceptably large test case even when the behavior is linear.

There are also a bunch of things that might "go quadratic" in this case. There are FIXMEs in the code for some of these things that I would like to address, but probably don't belong conflated into this patch...

Based on the example you post, I think I've figured out that you are trying to point out a case where we will run the SCC pass manager over the function E as many times as we successfully devirtualize edges somewhere in the SCC containing E in a way that brings a new node into that SCC. If I've understood this correctly, then I agree, and that's a nice find. I think it is unlikely to be a problem in practice, but it is definitely something we would need fixed to finish deploying this, probably with just a cap to limit things as a very large SCC formed in this way seems unlikely to be a practical concern to optimize heavily. Given that, I'm inclined to make a FIXME or note about this rather than trying to address it within this patch as that seems like it would bottleneck things.

Did I understand correctly? Does that approach make sense?


http://reviews.llvm.org/D21464