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

[Sean Silva via llvm-commits]

silvas added inline comments.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:528
@@ +527,3 @@
+      // Update the SCC if necessary.
+      C = UR.UpdatedC ? UR.UpdatedC : C;
+
----------------
chandlerc wrote:
> I'm not sure what you mean...
> 
> I think there are already test cases for invalidation, but I can double check. But that shouldn't be in *this* patch, that should be in the big CG update patch. I haven't gone back to that one to update it substantially (this iteration utility was requested and I wanted to get it out for review) but I'm happy to add test cases there, or add more testing in subsequent patches for that stack as problems are discovered.
> I think there are already test cases for invalidation, but I can double check. But that shouldn't be in *this* patch, that should be in the big CG update patch.

Totally agreed. Sorry if it wan't clear from context, but that's what I meant.

> I haven't gone back to that one to update it substantially (this iteration utility was requested and I wanted to get it out for review) but I'm happy to add test cases there, or add more testing in subsequent patches for that stack as problems are discovered.

That sounds reasonable. I'll add any comments there once the discussion reopens there.

================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:557
@@ +556,3 @@
+        // something.
+        if (!F->isIntrinsic()) {
+          Devirt = true;
----------------
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).


https://reviews.llvm.org/D23114