[PATCH] D23114: [PM] Introduce a devirtualization iteration layer for the new PM.

[Chandler Carruth via llvm-commits]

chandlerc marked 4 inline comments as done.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:557
@@ +556,3 @@
+        // something.
+        if (!F->isIntrinsic()) {
+          Devirt = true;
----------------
chandlerc wrote:
> silvas wrote:
> > chandlerc wrote:
> > > silvas wrote:
> > > > Can you just avoid keeping WeakVH's to intrinsics? The langref says you can't take their address. Therefore, a devirtualized call will never be to an intrinsic.
> > > > http://llvm.org/docs/LangRef.html#intrinsic-functions
> > > It's not that we could create a VH to a call to an instrinsic.
> > > 
> > > Above, we create a VH to an indirect call. That indirect call gets devirtualized into a direct call. At some point, some pass may turn that direct call into a call to an intrinsic, and if it does that this code needs to defensively prepare for it.
> > > 
> > > Sadly, there is no pass in tree that does this, but without this logic the pass would crash if anything ever conspired to trigger this.
> > I'm not sure I understand. In the situation you described regarding the intrinsic, we did in fact devirtualize something. So why are we avoiding setting `Devirt = true` in that case?
> > Is it because that doesn't actually add extra call edges to the lazy call graph? If that's the case, you need the same treatment for calls to external function declarations and calls to inlineasm too, right?
> > 
> > You may want to add some more comments about this (and maybe a test case (unittest?) if it isn't too verbose).
> Sorry, I explained this really poorly.
> 
> The idea I had was that if we end up with a call to an intrinsic, there is no longer any call graph or IPO refinement to motivate iteration: scalar passes can already fully reason about the intrinsic.
> 
> I've added a bunch of comments and restructured the code to try to make this more clear.
> 
> I'm not sure I can craft a test case that does this, but I'll take a stab at it...
Actually, got a test for this and it convinced me we shouldn't be special casing intrinsics. We can actually do very useful function attrs work here even when "devirtualizing" to an intrinsic that won't be inlined.

Even better, that test pointed out that this was missing a heuristic from the legacy PM's version: we don't track the change of count and have that also trigger iteration.

I've added the heuristic, removed any special casing of intrinsics, and have a nice test case that covers this logic.


https://reviews.llvm.org/D23114