[PATCH] D141190: [CallGraph][FIX] Ensure generic intrinsics are represented in the CG

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D141190#4037328 <https://reviews.llvm.org/D141190#4037328>, @nikic wrote:

> Can you please a) land the changes to patchpoint attributes and b) the conversion from isLeaf to !NoCallback separately, and then rebase this? Those two changes are obviously correct and desirable, but I'm not sure about the rest of the intrinsic intrinsic modelling here. It doesn't really seem the job of the call graph to model nocallback intrinsics?

a) Will do, also the test.
b) This is not as simple as there are two isLeaf uses that mean different things. See below.

W/o modeling intrinsics we try to make up for them in other places, e.g., GlobalsAA. That is arguably undesirable. It is also not about modeling nocallback intrinsics per se.
Right now, we add most intrinsics to the CG but only add *incoming* egdges for isLeaf intrinsics and never outgoing edges. Which means our CG is totally broken and mostly useless wrt. intrinsics.
Even the leaf ones are not represented reasonably because they are called but themselves call nothing (according to the CG). That works for some other magic reasons.

With this patch we add incoming and outgoing edges to intrinsic nodes where they make sense. So if the node is called, it has an incoming edge, if it can call back, it has an outgoing edge.
These are two separate changes and both needed to make the CG sane.



================
Comment at: llvm/lib/Analysis/CallGraph.cpp:37
   for (Function &F : M)
-    if (!isDbgInfoIntrinsic(F.getIntrinsicID()))
+    if (!isAssumeLikeIntrinsic(F.getIntrinsicID()))
       addToCallGraph(&F);
----------------
nikic wrote:
> So this means that for example `llvm.umax` is part of the call graph but `llvm.objectsize` is not -- the intended logic here isn't clear to me.
It is as clear as it was before. We added both of the above to the call graph, but not dbg stuff. And then we did not add a single edge connecting the intrinsics we added (except 3), which means adding them was a waste to begin with and totally unhelpful. I'll roll back to !isDbgInfo but that makes as little sense to me as the above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141190/new/

https://reviews.llvm.org/D141190